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

feat(webpack-cli): import flags from webpack core #1630

Merged
merged 21 commits into from
Jul 29, 2020

Conversation

snitin315
Copy link
Member

@snitin315 snitin315 commented Jun 14, 2020

What kind of change does this PR introduce?
Feature.

Did you add tests for your changes?
Yes
If relevant, did you update the documentation?
No
Summary

Follow up #1629
Refers #717
Closes #1241

Does this PR introduce a breaking change?

Other information

TODO

  • Write tests
  • remove duplicate flags
  • register --no-name negated flags.
  • fix help output ( currently showing type string for every flag )
  • allow parsing type: 'number'

@snitin315 snitin315 requested a review from a team as a code owner June 14, 2020 12:06
@snitin315 snitin315 marked this pull request as draft June 14, 2020 12:06
@snitin315
Copy link
Member Author

Right now works well -

Screenshot at 2020-06-15 09-42-48
Screenshot at 2020-06-15 09-43-09

@snitin315 snitin315 force-pushed the feat/core-flags branch 3 times, most recently from 5952f55 to 277b765 Compare June 16, 2020 04:11
@snitin315
Copy link
Member Author

snitin315 commented Jun 19, 2020

list of broken flags -

  • --amd fixed
  • --module-no-parse
  • --module-no-parse-reset
  • --module-rules-side-effect
  • --output-library-reset
  • --output-library-* related flags
  • --watch-options-poll

@snitin315
Copy link
Member Author

snitin315 commented Jun 20, 2020

Draft list of flags we can drop ( need discussion ) -

@anshumanv
Copy link
Member

I'm not sure of the feasibility of removing this on the core, should we keep a list of unsupported flags and remove them after importing core flags?

package.json Outdated Show resolved Hide resolved
@snitin315
Copy link
Member Author

I'm not sure of the feasibility of removing this on the core, should we keep a list of unsupported flags and remove them after importing core flags?

Yes, I think we can filter flags on the CLI side.

@snitin315
Copy link
Member Author

--amd is broken on our side. it accepts only false value and right now CLI doesn't support parsing of boolean flags like webpack-cli --amd=false.
Looking into it if it's possible to parse it like this with commander.js

@alexander-akait
Copy link
Member

alexander-akait commented Jun 24, 2020

@snitin315

--amd is broken on our side. it accepts only false value and right now CLI doesn't support parsing of boolean flags like webpack-cli --amd=false.

Not sure we should support it, because we have --amd and --no-amd support, boolean as string maybe some misleading, for example --entry=false can be mean:

  • loading false.js as entry point
  • no entry configuration (possible with webpack@5)

What we should use? I don't know, so better:

  • --no-entry means no entry
  • --entry=false uses false.js for entry

@snitin315
Copy link
Member Author

@evilebottnawi I think we can specify --no-amd in cli-flags and we will be good.

@alexander-akait
Copy link
Member

@snitin315 For all (not sure for all but for most of them) boolean flag we should register --no-name flag

@snitin315
Copy link
Member Author

Let's implement --no-name flag for all now, we can filter them later (if needed).

@alexander-akait
Copy link
Member

@snitin315 Can we move these changes:

  • allow parsing type: 'number'
  • fix help output ( currently showing type string for every flag )

to other PRs, to avoid unnecessary changes in this PR?

rishabh3112
rishabh3112 previously approved these changes Jul 29, 2020
Copy link
Member

@rishabh3112 rishabh3112 left a comment

Choose a reason for hiding this comment

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

Looks good.

anshumanv
anshumanv previously approved these changes Jul 29, 2020
Copy link
Member

@anshumanv anshumanv left a comment

Choose a reason for hiding this comment

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

💯

@ghost
Copy link

ghost commented Jul 29, 2020

There were the following issues with this Pull Request

  • Commit: a006725
    • ✖ message may not be empty
    • ✖ type may not be empty

You may need to change the commit messages to comply with the repository contributing guidelines.


🤖 This comment was generated by commitlint[bot]. Please report issues here.

Happy coding!

1 similar comment
@ghost
Copy link

ghost commented Jul 29, 2020

There were the following issues with this Pull Request

  • Commit: a006725
    • ✖ message may not be empty
    • ✖ type may not be empty

You may need to change the commit messages to comply with the repository contributing guidelines.


🤖 This comment was generated by commitlint[bot]. Please report issues here.

Happy coding!

@webpack-bot
Copy link

@jamesgeorge007 Thanks for your update.

I labeled the Pull Request so reviewers will review it again.

@anshumanv Please review the new changes.

@anshumanv
Copy link
Member

@jamesgeorge007 please don't commit yourself, suggestions suffice

@alexander-akait
Copy link
Member

@webpack/cli-team Merge?

@anshumanv
Copy link
Member

@webpack/cli-team Merge?

yep we can merge 👍

@alexander-akait alexander-akait merged commit f478cc9 into webpack:next Jul 29, 2020
@alexander-akait
Copy link
Member

Big thanks for all, it is great feature, I think we should resolve serve command and do release, when focus on perf and refactor

@snitin315 snitin315 deleted the feat/core-flags branch July 29, 2020 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Import flags from webpack core
7 participants