feat: middleware #881

Khaledgarbaya · 2017-05-24T08:52:54Z

OK this is a very WIP and ugly first draft of the middleware feature.
I will need some help to change passing the middleware using a chaining function instead of an array.
For more context check #876

@bcoe I would love to get some feedback from you.

TODO :

Add documentation
Add more tests
Cleanup
Rebase and squash commits

bcoe · 2017-05-26T03:02:47Z

@Khaledgarbaya thanks for opening this pull! I have a lot of OSS time this weekend and will give feedback.

bcoe · 2017-05-27T21:29:37Z

@Khaledgarbaya one thing that's been on my mind, before we extend yargs with too many more features, is I'd love to split the massive README.md in the root folder into a few docs in a ./docs folder, I was thinking something along the lines of:

docs/examples.
docs/basics.
docs/advanced-features.
docs/commands.

Haven't fully thought it out yet -- but would like to perform this decomposition in the fairly near future as we build out some more features.

Khaledgarbaya · 2017-06-09T10:53:28Z

@bcoe I guess we can wait until the restructuring PR is done and we can discuss this PR. I have no problem with this.

bcoe · 2017-06-10T20:04:20Z

@Khaledgarbaya sorry 👋 have been pretty busy juggling various other works (working on some pretty cool things at npm 😄 ).

I've taken a stab at restructuring the README over the past week, and would love your feedback:

#892

What section would this middleware fall under do you think.

My only real concern with the pull request currently, is that the command() method ends up taking a lot of arguments -- this is partially my fault for already having overloaded the method so much -- I wonder if there's any workaround for this ... is there any other way that one could reasonably add middleware to a command() that would feel natural; maybe it does have to be the fourth argument.

Khaledgarbaya · 2017-06-11T12:44:04Z

hi @bcoe I was thinking of that too recently, I am thinking of two ways.

is to have a map at yargs level that links middlewares to a specific command.
e.g.

var argv = require('yargs')
    .usage('Usage: $0 <command> [options]')
    .command('count', 'Count the lines in a file')
    .middlewares({'count': [middleware1, middlware2]})
    .argv;

or have the use middlewares of the command

var argv = require('yargs')
    .usage('Usage: $0 <command> [options]')
    .command('count', 'Count the lines in a file')
    .middlewares([middleware1, middlware2])
    .argv;

First option will let us to define middlewares that can run before each command for example if we specify '*'. Second option is not bad too but if we follow the current implementation of yargs middlewares should follow the same patterns as aliases etc .. meaning you should be able to pass it as property if you go the object route or export it inside the command module if you use commandDir.

In conclusion option 2 will still introduce a new argument to the command and option 1 will reduce the number of arguments by one and give us the possibility to run the same middleware for all the commands if we want.

Given the structure of yargs I am leaning tward solution 1 since it won't involve a lot of changes to the library and in the future maybe we can improve the command creation.

To answer the documentation question I think middlewares should be in the advanced section. I can add that once the docs are merged.

Best,
Khaled

bcoe · 2017-07-04T19:11:40Z

👋 hey @Khaledgarbaya sorry that I've been dropping this on the floor for so long, I've been on a couple trips this summer, and have also been dealing with some family issues

long story short haven't had quite as many hours to work on OSS as I would like.

Now that I've refactored the docs for yargs, why don't you take a stab at adding a new advanced topic document on middleware, with a few practical examples; I think we can center the discussion of the optimal API around this document, and then work on getting this landed.

With yargs I've tried to take a slow methodical approach to evolving the API, which I understand can be a bit frustrating -- I promise we can get this landed though 👍

Khaledgarbaya · 2017-07-09T18:10:50Z

hey, @bcoe, no worries I was also on vacation and I will definitely try to find to move this PR forward.

bcoe · 2017-07-11T05:29:00Z

lib/command.js

@@ -3,7 +3,7 @@ const inspect = require('util').inspect
 const camelCase = require('camelcase')

 const DEFAULT_MARKER = '*'
-
+const objectAssign = require('object-assign')


now that we require >=Node 4, you should be able to just use Object.assign.

bcoe · 2017-08-15T04:11:38Z

@Khaledgarbaya a good way to get this slick feature over the finish line would be to join the community slack that I recently created:

http://devtoolscommunity.herokuapp.com/

would love to coordinate with you, and help you land this.

Khaledgarbaya · 2017-09-03T12:24:37Z

Hey @bcoe, I made some changes and added the docs Sorry for taking so long

bcoe · 2017-09-03T21:26:28Z

docs/advanced.md

+
+```
+                        --------------         --------------        ---------
+stdin ----> argv ----> | Middleware 1 | ----> | Middleware 2 | ---> | Command |


love this diagram 👍

bcoe

This is looking great. Your implementation is really slick, it's awesome that it's ultimately basically 5 lines of code 😛

Just a few comments, but I think this is almost ready to land!

bcoe · 2017-09-03T21:28:16Z

docs/advanced.md

+  .middlewares([normalizeCredentials])
+  .argv;
+```
+

 See the [yargs-parser](https://github.com/yargs/yargs-parser#configuration) module


this section should be moved up to be with the "Customizing Yargs' Parser" section.

bcoe · 2017-09-03T21:30:06Z

test/yargs.js

@@ -301,7 +301,26 @@ describe('yargs dsl tests', () => {
        .exitProcess(false) // defaults to true.
        .argv
    })
-
+    it('run middlewares before reaching the handler', function (done) {


slight grammar nit: "it('runs all middleware before reaching the handler'").

bcoe · 2017-09-03T21:30:54Z

test/yargs.js

+          function (argv) {
+            // we should get the argv filled with data from the middleware
+            argv._[0].should.equal('foo')
+            console.log(argv)


we should get rid of this console.log.

bcoe · 2017-09-03T21:32:11Z

test/yargs.js

+            return done()
+          },
+        [function (argv) {
+          return {hello: 'world'}


slick, so middleware is additive? we should definitely make sure we mention that it can both add new keys (while maintaining the existing) or overwrite old keys.

bcoe · 2017-09-03T21:32:27Z

docs/advanced.md

@@ -403,6 +403,51 @@ some of yargs' parsing features:
  }
 }
 ```
+## Midlewares


Let's just make this heading: "Middleware".

bcoe · 2017-09-03T21:36:55Z

docs/advanced.md

+### Usage
+
+Here the middleware will check if the `username` and `password` is passed by the user
+if not it will load them from `~./credential` file and fil in the argv.username and argv.password.


slight grammar nits:

In this example, our middleware will check if the `username` and `password` is provided. If not, it will load them from `~/.credentials`, and fill in the `argv.username` and `argv.password` values.

bcoe · 2017-09-03T21:37:11Z

docs/advanced.md

+
+Here the middleware will check if the `username` and `password` is passed by the user
+if not it will load them from `~./credential` file and fil in the argv.username and argv.password.
+This means in your command you are sure that `argv.username` and `argv.password` have values in them.


you can probably drop this sentence (I think it's clear enough).

bcoe · 2017-09-03T21:37:30Z

docs/advanced.md

+}
+```
+
+#### yarg


"""#### yargs parsing configuration"""

bcoe · 2017-09-03T21:37:40Z

docs/advanced.md

+if not it will load them from `~./credential` file and fil in the argv.username and argv.password.
+This means in your command you are sure that `argv.username` and `argv.password` have values in them.
+
+#### middlware


"#### middleware function"

bcoe · 2017-09-03T21:38:47Z

lib/command.js

@@ -223,6 +223,12 @@ module.exports = function command (yargs, usage, validation) {

    if (commandHandler.handler && !yargs._hasOutput()) {
      yargs._setHasOutput()
+      if (commandHandler.middlewares.length > 0) {
+        const middlewareArgs = commandHandler.middlewares.reduce(function (initialObj, middleware) {
+          return Object.assign(initialObj, middleware(innerArgv))


to give people the ability to remove an argument in middleware, I think we should perhaps give middleware the actual object to modify, rather than modifying by reference with Object.assign, then a middleware function could:

delete argv.password and it would not show up in the final argv passed to the command. What do you think?

@bcoe In this case, we would change how the middleware rather than being additive, it changes the actual argv object. I didn't want to touch the exact object at first but if you don't see any side effects when editing the ref I can change that.

bcoe · 2017-09-29T00:56:01Z

@Morishiri interested in providing feedback on this feature? I'd love to unblock @Khaledgarbaya.

Morishiri · 2017-09-29T15:34:46Z

I just took a look at it and it looks really nice, interesting feature, really usefull :) Fully approved

bcoe · 2017-11-23T20:18:42Z

@Khaledgarbaya thank you for the awesome contribution 🎉

bcoe · 2018-01-01T23:08:18Z

@Khaledgarbaya, @Darkylin please try npm i yargs@next, which has support for middleware. If everything is looking good I will promote the release.

jacobrosenthal · 2018-01-03T05:57:55Z

Im on yargs@next and Im attempting to use middleware as documented

const normalizeCredentials = (argv) => {
  if (!argv.username || !argv.password) {
    const credentials = JSON.parse(fs.readSync('~/.credentials'))
    return credentials
  }
  return {}
}

var argv = require('yargs')
  .usage('Usage: $0 <command> [options]')
  .command('login', 'Authenticate user', (yargs) =>{
        return yargs.option('username')
                    .option('password')
      } ,(argv) => {
        authenticateUser(argv.username, argv.password)
      })
  .middlewares([normalizeCredentials])
  .argv;

but I get

bash-3.2$ node index.js --help
/Users/jacobrosenthal/Downloads/node-cryptokitties-cli/index.js:17
  .middlewares([normalizeCredentials])
   ^

TypeError: require(...).usage(...).command(...).middlewares is not a function
    at Object.<anonymous> (/Users/jacobrosenthal/Downloads/node-cryptokitties-cli/index.js:17:4)
    at Module._compile (module.js:571:32)
    at Object.Module._extensions..js (module.js:580:10)
    at Module.load (module.js:488:32)
    at tryModuleLoad (module.js:447:12)
    at Function.Module._load (module.js:439:3)
    at Module.runMain (module.js:605:10)
    at run (bootstrap_node.js:427:7)
    at startup (bootstrap_node.js:151:9)
    at bootstrap_node.js:542:3
bash-3.2$

Possible documentation or implementation error?

bcoe · 2018-01-03T06:25:40Z

@jacobrosenthal I believe this is an issue with documentation, @Khaledgarbaya ultimately decided that it made the most sense to pass middleware as the last argument to a .command(), rather than having a separate middlewares option.

@Khaledgarbaya, am I correct in this assertion? mind updating the docs when you have a moment?

Khaledgarbaya · 2018-01-03T09:58:22Z

Hey @bcoe @jacobrosenthal
I am terribly sorry about that It totally slipped through.
I made a PR to fix that #1037

jacobrosenthal · 2018-01-06T20:41:17Z

That all worked great btw. However one follow up question. Is there any benefit of using the middleware argument vs calling a function (or two functions) in the command builder slot as far as internal lifecycle or something?

bcoe · 2018-01-09T07:06:56Z

@jacobrosenthal I think @Khaledgarbaya's idea was that you'd be able to apply a complex stack of transforms to all arguments in argv prior to it getting passed to the command handler...

You might be able to get similar behavior with a builder for individual arguments, whereas middleware processes the final argv object. @Khaledgarbaya out of curiosity do you have any real world examples of using this feature you can point us towards yet.

bcoe self-requested a review July 7, 2017 18:33

bcoe reviewed Jul 11, 2017

View reviewed changes

bcoe added the triaged label Aug 15, 2017

bcoe assigned Khaledgarbaya Aug 15, 2017

Khaledgarbaya force-pushed the feat/middlewares branch from 3d0cf30 to 1bea82d Compare September 3, 2017 11:52

bcoe mentioned this pull request Sep 3, 2017

Is there some way to stop parsing at a given point or just collect all remaining text? #946

Closed

bcoe reviewed Sep 3, 2017

View reviewed changes

bcoe requested changes Sep 3, 2017

View reviewed changes

bcoe mentioned this pull request Sep 17, 2017

[feature] add optional errorHandler #955

Closed

Khaledgarbaya added 2 commits November 23, 2017 20:13

feat(yargs): Add middlewares to commands

d4659a2

Change to arr reduce

8f996e8

Khaledgarbaya force-pushed the feat/middlewares branch from 2e4126f to 813b20f Compare November 23, 2017 19:15

Khaledgarbaya and others added 5 commits November 23, 2017 20:19

fix: Array.reduce

a732828

Add docs

0ac7651

Change Object assign

b14b6cc

chore: Update Docs

1a8dc7f

Test: Update test

f28561d

Khaledgarbaya force-pushed the feat/middlewares branch from 813b20f to f28561d Compare November 23, 2017 19:19

test: fix test

3f07809

bcoe approved these changes Nov 23, 2017

View reviewed changes

bcoe changed the title ~~[WIP] Feature middlewares~~ feat: middleware Nov 23, 2017

bcoe merged commit 77b8dbc into yargs:master Nov 23, 2017

Khaledgarbaya deleted the feat/middlewares branch November 23, 2017 20:26

This was referenced Apr 22, 2018

PR-AN-01: Global Middleware feature. #1109

Closed

Feature Request: (PR-AN-01) Global middleware #1110

Closed

This was referenced Nov 25, 2021

chore(main): release yargs 1.0.0 #2072

Closed

chore(main): release yargs 1.0.0 #2073

Closed

chore(main): release 1.0.0 #2082

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: middleware #881

feat: middleware #881

Khaledgarbaya commented May 24, 2017 •

edited

bcoe commented May 26, 2017

bcoe commented May 27, 2017

Khaledgarbaya commented Jun 9, 2017

bcoe commented Jun 10, 2017 •

edited

Khaledgarbaya commented Jun 11, 2017 •

edited

bcoe commented Jul 4, 2017

Khaledgarbaya commented Jul 9, 2017

bcoe Jul 11, 2017

bcoe commented Aug 15, 2017

Khaledgarbaya commented Sep 3, 2017

bcoe Sep 3, 2017

bcoe left a comment

bcoe Sep 3, 2017

bcoe Sep 3, 2017

bcoe Sep 3, 2017

bcoe Sep 3, 2017

bcoe Sep 3, 2017

bcoe Sep 3, 2017

bcoe Sep 3, 2017

bcoe Sep 3, 2017

bcoe Sep 3, 2017

bcoe Sep 3, 2017

Khaledgarbaya Sep 7, 2017

bcoe commented Sep 29, 2017

Morishiri commented Sep 29, 2017

bcoe commented Nov 23, 2017

bcoe commented Jan 1, 2018

jacobrosenthal commented Jan 3, 2018

bcoe commented Jan 3, 2018

Khaledgarbaya commented Jan 3, 2018

jacobrosenthal commented Jan 6, 2018

bcoe commented Jan 9, 2018

feat: middleware #881

feat: middleware #881

Conversation

Khaledgarbaya commented May 24, 2017 • edited

bcoe commented May 26, 2017

bcoe commented May 27, 2017

Khaledgarbaya commented Jun 9, 2017

bcoe commented Jun 10, 2017 • edited

Khaledgarbaya commented Jun 11, 2017 • edited

bcoe commented Jul 4, 2017

Khaledgarbaya commented Jul 9, 2017

Choose a reason for hiding this comment

bcoe commented Aug 15, 2017

Khaledgarbaya commented Sep 3, 2017

Choose a reason for hiding this comment

bcoe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcoe commented Sep 29, 2017

Morishiri commented Sep 29, 2017

bcoe commented Nov 23, 2017

bcoe commented Jan 1, 2018

jacobrosenthal commented Jan 3, 2018

bcoe commented Jan 3, 2018

Khaledgarbaya commented Jan 3, 2018

jacobrosenthal commented Jan 6, 2018

bcoe commented Jan 9, 2018

Khaledgarbaya commented May 24, 2017 •

edited

bcoe commented Jun 10, 2017 •

edited

Khaledgarbaya commented Jun 11, 2017 •

edited