New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: middleware #881

Merged
merged 8 commits into from Nov 23, 2017

Conversation

Projects
None yet
4 participants
@Khaledgarbaya
Contributor

Khaledgarbaya commented May 24, 2017

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

This comment has been minimized.

Member

bcoe commented May 26, 2017

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

@bcoe

This comment has been minimized.

Member

bcoe commented May 27, 2017

@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

This comment has been minimized.

Contributor

Khaledgarbaya commented Jun 9, 2017

@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

This comment has been minimized.

Member

bcoe commented Jun 10, 2017

@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

This comment has been minimized.

Contributor

Khaledgarbaya commented Jun 11, 2017

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

  1. 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;
  1. 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

This comment has been minimized.

Member

bcoe commented Jul 4, 2017

👋 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 👍

@bcoe bcoe self-requested a review Jul 7, 2017

@Khaledgarbaya

This comment has been minimized.

Contributor

Khaledgarbaya commented Jul 9, 2017

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

@@ -3,7 +3,7 @@ const inspect = require('util').inspect
const camelCase = require('camelcase')
const DEFAULT_MARKER = '*'
const objectAssign = require('object-assign')

This comment has been minimized.

@bcoe

bcoe Jul 11, 2017

Member

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

@bcoe

This comment has been minimized.

Member

bcoe commented Aug 15, 2017

@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.

@bcoe bcoe added the triaged label Aug 15, 2017

@Khaledgarbaya Khaledgarbaya force-pushed the Khaledgarbaya:feat/middlewares branch from 3d0cf30 to 1bea82d Sep 3, 2017

@Khaledgarbaya

This comment has been minimized.

Contributor

Khaledgarbaya commented Sep 3, 2017

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

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

This comment has been minimized.

@bcoe

bcoe Sep 3, 2017

Member

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!

.middlewares([normalizeCredentials])
.argv;
```
See the [yargs-parser](https://github.com/yargs/yargs-parser#configuration) module

This comment has been minimized.

@bcoe

bcoe Sep 3, 2017

Member

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

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

This comment has been minimized.

@bcoe

bcoe Sep 3, 2017

Member

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

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

This comment has been minimized.

@bcoe

bcoe Sep 3, 2017

Member

we should get rid of this console.log.

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

This comment has been minimized.

@bcoe

bcoe Sep 3, 2017

Member

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.

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

This comment has been minimized.

@bcoe

bcoe Sep 3, 2017

Member

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

### 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.

This comment has been minimized.

@bcoe

bcoe Sep 3, 2017

Member

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.
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.

This comment has been minimized.

@bcoe

bcoe Sep 3, 2017

Member

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

}
```
#### yarg

This comment has been minimized.

@bcoe

bcoe Sep 3, 2017

Member

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

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

This comment has been minimized.

@bcoe

bcoe Sep 3, 2017

Member

"#### middleware function"

@@ -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))

This comment has been minimized.

@bcoe

bcoe Sep 3, 2017

Member

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?

This comment has been minimized.

@Khaledgarbaya

Khaledgarbaya Sep 7, 2017

Contributor

@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

This comment has been minimized.

Member

bcoe commented Sep 29, 2017

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

@Morishiri

This comment has been minimized.

Collaborator

Morishiri commented Sep 29, 2017

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

@Khaledgarbaya Khaledgarbaya force-pushed the Khaledgarbaya:feat/middlewares branch from 2e4126f to 813b20f Nov 23, 2017

@Khaledgarbaya Khaledgarbaya force-pushed the Khaledgarbaya:feat/middlewares branch from 813b20f to f28561d Nov 23, 2017

@bcoe

bcoe approved these changes Nov 23, 2017

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

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

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.2%) to 100.0%
Details
@bcoe

This comment has been minimized.

Member

bcoe commented Nov 23, 2017

@Khaledgarbaya thank you for the awesome contribution 🎉

@Khaledgarbaya Khaledgarbaya deleted the Khaledgarbaya:feat/middlewares branch Nov 23, 2017

@bcoe

This comment has been minimized.

Member

bcoe commented Jan 1, 2018

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

@jacobrosenthal

This comment has been minimized.

jacobrosenthal commented Jan 3, 2018

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

This comment has been minimized.

Member

bcoe commented Jan 3, 2018

@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

This comment has been minimized.

Contributor

Khaledgarbaya commented Jan 3, 2018

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

@jacobrosenthal

This comment has been minimized.

jacobrosenthal commented Jan 6, 2018

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

This comment has been minimized.

Member

bcoe commented Jan 9, 2018

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment