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

Fix validation error with default uglify options #231

Merged
merged 4 commits into from
Oct 26, 2017

Conversation

jvanbruegge
Copy link
Contributor

When running a webpack config with the default options the build fails due to a validation error.

@andywer andywer requested a review from sapegin October 10, 2017 11:36
@sapegin
Copy link
Collaborator

sapegin commented Oct 10, 2017

@jvanbruegge
Copy link
Contributor Author

I dont think so, but the validation is not throwing an error

@sapegin
Copy link
Collaborator

sapegin commented Oct 10, 2017

Then if boolean for parallel option is incorrect we should send a PR to uglifyjs-webpack-plugin too.

@sapegin
Copy link
Collaborator

sapegin commented Oct 10, 2017

webpack-contrib/uglifyjs-webpack-plugin#139

@sapegin
Copy link
Collaborator

sapegin commented Oct 10, 2017

They’ve made a breaking change without any change log entries, the correct settings are:

parallel: true,
cache: true,

@sapegin
Copy link
Collaborator

sapegin commented Oct 10, 2017

We should also lock the uglifyjs-webpack-plugin version in package.json.

@andywer
Copy link
Owner

andywer commented Oct 10, 2017

Thx for investigating, @sapegin! 👍

@jvanbruegge jvanbruegge force-pushed the uglify-options branch 2 times, most recently from 00252bf to 2d014b9 Compare October 11, 2017 16:54
sapegin
sapegin previously approved these changes Oct 16, 2017
@sapegin
Copy link
Collaborator

sapegin commented Oct 16, 2017

Now the question is: why the CI is failing?

@sapegin
Copy link
Collaborator

sapegin commented Oct 16, 2017

@andywer Any ideas?

@jvanbruegge
Copy link
Contributor Author

No idea

@andywer
Copy link
Owner

andywer commented Oct 16, 2017

@sapegin Don't know. Just restarted the failed build. Maybe some issue with node 6 vs. node 8.

@andywer
Copy link
Owner

andywer commented Oct 16, 2017

An end-to-end test fails and maybe lerna swallows the error message.

@vlad-zhukov
Copy link
Collaborator

v1.0 was released

@andywer
Copy link
Owner

andywer commented Oct 25, 2017

@vlad-zhukov Thx for the hint! 🙂

@andywer
Copy link
Owner

andywer commented Oct 26, 2017

@jvanbruegge Can we just merge jvanbruegge#1? Then we should be able to merge this PR, release a new version of the uglify block and the other PT builds should become green as well :)

Would just be good if someone would have a look at my changes in jvanbruegge#1

@jvanbruegge
Copy link
Contributor Author

Done

Copy link
Collaborator

@vlad-zhukov vlad-zhukov 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, but I'd put the sample app into an examples folder and add it to lerna rather than keep it in packages. It's not critical and we can do it in another PR.

@andywer
Copy link
Owner

andywer commented Oct 26, 2017

Will merge and release this evening :)

@andywer andywer merged commit 8781129 into andywer:master Oct 26, 2017
@andywer
Copy link
Owner

andywer commented Oct 26, 2017

@vlad-zhukov

I'd put the sample app into an examples folder and add it to lerna

How would you add it to lerna?

@vlad-zhukov
Copy link
Collaborator

@andywer using the packages field. I can submit a PR tomorrow.

@andywer
Copy link
Owner

andywer commented Oct 26, 2017

@vlad-zhukov Ahh, alright. Good idea 👍

@andywer
Copy link
Owner

andywer commented Oct 26, 2017

Published as @webpack-blocks/uglify v1.1.0 & webpack-blocks v1.0.0-rc.2 🚀

@vlad-zhukov vlad-zhukov mentioned this pull request Oct 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants