Skip to content
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

Better CLI #121

Closed
wants to merge 5 commits into from
Closed

Better CLI #121

wants to merge 5 commits into from

Conversation

okonet
Copy link
Contributor

@okonet okonet commented May 4, 2017

What kind of change does this PR introduce?
feature / refactoring

Did you add tests for your changes?

No, we need to introduce e2e tests for that

If relevant, did you update the documentation?

Yes

Summary

Refactors the CLI code so we use commands for migrate and init.
Made use of yargs to simplify the source code.
Consolidated all webpack options into one file.

Does this PR introduce a breaking change?
Yes, the commands should be now called with webpack-cli migrate instead of webpack-cli --migrate

Other information

okonet added 2 commits May 4, 2017 23:26
- Rename migrate and init options to command. Closes #89
- Use `commands` from yargs
- Add start command and made it default
- Move all options to one place
- Show options only for start command
@okonet okonet self-assigned this May 4, 2017
@okonet okonet requested a review from evenstensberg May 4, 2017 21:29
@okonet okonet added this to the v1 milestone May 4, 2017
@okonet okonet changed the base branch from master to feature/init-ast May 4, 2017 21:43
@okonet okonet changed the base branch from feature/init-ast to master May 8, 2017 08:01
@okonet okonet requested a review from sokra May 8, 2017 08:02
@evenstensberg
Copy link
Member

Ready for review @okonet ?

@okonet
Copy link
Contributor Author

okonet commented May 8, 2017

Almost there.

@okonet
Copy link
Contributor Author

okonet commented May 8, 2017

So after spending a good amount of time on this, I reached a dead end it seems. I can't think of a good way of doing this change as a non-breaking change to the existing webpack CLI.

The problem is that current CLI syntax looks like this webpack <entry> [<entry>] <output>.

Making migrate and init tasks act as commands means, the syntax to use them will be webpack init which might be ambiguous to the current webpack CLI.

Another issue I tried to solve is to not display all options by default when running webpack --help. This also seems impossible with a non-breaking change.

A breaking change means an introduction of an additional command for webpack default behavior. For example compile or start.

There is probably a solution for this that involves parsing of arguments but I'm not sure yet how reliable this can be implemented. Any feedback is appreciated!

@sokra I think you should be involved in this discussion since implications can be pretty big.

@TheLarkInn
Copy link
Member

Any good ideas @sokra

@okonet
Copy link
Contributor Author

okonet commented May 13, 2017

We discussed on Slack but I'm a bit drawn by work now so I'll continue on this in a week or so.

Usage without config file: webpack <entry> [<entry>] <output>
Usage with config file: webpack
`)
.epilogue('for more information see https://webpack.github.io/docs/cli.html')
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should point to new docs?

@DanielaValero
Copy link
Contributor

@okonet anything here for me to help with?

@okonet
Copy link
Contributor Author

okonet commented Jul 3, 2017

More free time would be great :( Seriously though, having tests would help dramatically before I refactor even further. Not sure how much of this is covered.

@evenstensberg
Copy link
Member

@okonet we should get that react test suite up and running first, yea?

@okonet
Copy link
Contributor Author

okonet commented Sep 8, 2017

What react test suit? You mean Jest snapshot tests? yes, that would be great to have. We need 100% coverage if we don't want break current implementation.

@evenstensberg
Copy link
Member

@okonet You mentioned on Slack that instead of Soren, you could use this https://github.com/vadimdemedes/ink for testing

@okonet
Copy link
Contributor Author

okonet commented Sep 8, 2017

@ev1stensberg it's not for testing but for the CLI itself and I'd like to look into it.

@evenstensberg
Copy link
Member

@okonet gotcha! We would need to come up with a way to test E2E and produce results, both for the new features but also the "old" CLI commands that webpack have.

@evenstensberg
Copy link
Member

May be better to redo this in another fresh PR after its been merged with webpack, closing @okonet , reopen if you want to continue to work on this branch!

@ematipico ematipico deleted the feature/commands branch April 8, 2018 07:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants