-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
Rollup update #6200
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
Rollup update #6200
Conversation
|
@duailibe pipelines not working? |
|
@fisker should be. Maybe it's because of merge conflicts in |
Does not sound like an acceptable change to me unless Prettier does this in a new major release, unfortunately. (And I think there is some reluctance to make a new major release unless absolutely necessary at this point.)
|
|
@duailibe I tried to update from master, still conflicts |
|
@brainkim I understand we should ES5, just too much work for this update, I temp use |
|
@fisker Did you update master? |
@brodybits we already run tests with the final bundle in Node 4 |
|
@duailibe I saw you changed the sources, so external can't work, right? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are you sure this is safe?
Iook like rollup-plugin-babel is not working as expected
hope not breaking anything
this comment is about uglify replace with terser
Can you expand? |
As I checked, before |
|
seems sorry, not well tested |
|
Yeah, we can't minify postcss-parser. I'm not sure why |
I'm not sure why it didn't work, but the code is ES5 and is tested in Node 4. |
|
Thank you @fisker! Great job! |
|
@duailibe the |
yes, this is why I start this pr, looks code is right, so I let you decide |
Why?
That's unlikely.. ideally we wouldn't even need the |
|
about ES5 check @brodybits mentioned before, I think there is a |
|
Node 4 supports a lot of ES6 stuff. See https://node.green/ |
|
@thorn0 You're right. It's not supposed to be ES5, it just needs to work in Node 4. |
|
@duailibe Which means we can't expect uglify to parse it. |
|
@fisker Yes, it's for browser. We don't necessarily need to support old browsers. We use whatever is matched by |
|
why node 4? for node I think 8 is good enough |
|
@thorn0 Makes sense. But it seems terser should be able to minify anything uglify also minifies, so we're safe. The minify step is not supposed to be a check if the syntax is correct. |
|
@duailibe problem is babel seem not working right. need more test before release |
|
@fisker Ok, I didn't see any problem with babel. When you find out, let me know. |
docs/directory)CHANGELOG.unreleased.mdfile following the template.✨Try the playground for this PR✨
fix #6154
FYI:
uglifyis replaced withterser, some es6 code come up in bundletests_integrationis using different export, hope it's not a problem