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

Make use of webpack-cli in webpack #5869

Merged
merged 11 commits into from Dec 11, 2017
Merged

Make use of webpack-cli in webpack #5869

merged 11 commits into from Dec 11, 2017

Conversation

evenstensberg
Copy link
Member

@sokra this is ready for a final review now. @shellscape needs to verify it doesn’t break the dev-server

@evenstensberg
Copy link
Member Author

Facebook aren't willing to upgrade jscodeshift to get rid of the warning ( see: facebook/jscodeshift#198 (comment) )

@shellscape
Copy link
Contributor

@ev1stensberg i'll prob have a chance to check this out on 10/22 EDT. the one big thing we'll have to look for is the options validation stuff, which WDS piggybacks on.

Copy link
Member

@sokra sokra left a comment

Choose a reason for hiding this comment

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

Could you not remove the binCases in this PR?

These tests should continue to work when webpack-cli is added.

@evenstensberg
Copy link
Member Author

If you've hardcoded some logic in the dev-server, the CLI has a lot of the same logic where it used to be @shellscape

@evenstensberg
Copy link
Member Author

@sokra any idea why the tests are failing? Might be https://github.com/webpack/webpack-cli/blob/master/bin/webpack.js#L28-L32

@evenstensberg
Copy link
Member Author

skjermbilde 2017-10-27 kl 00 09 13 weird..

@evenstensberg
Copy link
Member Author

@sokra Tests should run fine now, added a .nsprc to avoid breaking the build every time.

@evenstensberg
Copy link
Member Author

@shellscape webpack/webpack-dev-server#1163

Anything else that needs attention?

Copy link
Member

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

.nsprc Outdated
@@ -0,0 +1,3 @@
{
"exceptions": ["https://nodesecurity.io/advisories/118"]
Copy link
Member

Choose a reason for hiding this comment

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

Is this fixes? Could you remove this file?

Copy link
Member Author

Choose a reason for hiding this comment

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

it will make the nsp check succeed

@evenstensberg
Copy link
Member Author

@sokra what do you wanna do about the security issue? Adding a nsprc will make the CI pass

@evenstensberg
Copy link
Member Author

@sokra CLI is resynced against the next branch again. Before merging, any potential changes to binTestCases or the /bin folder, could you also redirect them to the CLI repo, or chill them until the CLI gets merged? Avoids extra refactoring work

@sokra
Copy link
Member

sokra commented Dec 6, 2017

Before merging, any potential changes to binTestCases or the /bin folder, could you also redirect them to the CLI repo, or chill them until the CLI gets merged? Avoids extra refactoring work

I try to.

@sokra
Copy link
Member

sokra commented Dec 8, 2017

After a long fight js-codeshift was updated...

So we can merge this PR once dependencies are updated in webpack-cli resp. lockfiles in this PR.

@webpack-bot
Copy link
Contributor

@ev1stensberg Thanks for your update.

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

@sokra Please review the new changes.

@webpack-bot
Copy link
Contributor

Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon.

@evenstensberg
Copy link
Member Author

CI build failed on yarn/cache, but I think this is ready now @sokra

@sokra
Copy link
Member

sokra commented Dec 11, 2017

Awesome ❤️

@sokra sokra merged commit e8bfcd7 into webpack:next Dec 11, 2017
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.

None yet

5 participants