Skip to content

Conversation

@fisker
Copy link
Member

@fisker fisker commented Jun 7, 2019

  • I’ve added tests to confirm my change works.
  • (If changing the API or CLI) I’ve documented the changes I’ve made (in the docs/ directory)
  • (If the change is user-facing) I’ve added my changes to the CHANGELOG.unreleased.md file following the template.
  • I’ve read the contributing guidelines.

Try the playground for this PR

fix #6154

FYI:

  1. uglify is replaced with terser, some es6 code come up in bundle
  2. third-party external solution seem a little bit weird, but working for now, can't find a better way
  3. thirdParty in tests_integration is using different export, hope it's not a problem

@fisker fisker changed the title Rollup update [fix #6154] Rollup update Jun 7, 2019
@fisker
Copy link
Member Author

fisker commented Jun 7, 2019

@duailibe pipelines not working?

@duailibe
Copy link
Collaborator

duailibe commented Jun 7, 2019

@fisker should be. Maybe it's because of merge conflicts in yarn.lock?

@brody4hire
Copy link

brody4hire commented Jun 7, 2019

[...] some es6 code come up in bundle

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.)

I wonder if we should use some kind of tool that automatically verifies ES5 compliance, and look for some kind of simple solution for whatever cases may come up? Not needed since tests are already run on Node.js version 4, as stated below by @duailibe. My bad, apologies.

@fisker
Copy link
Member Author

fisker commented Jun 7, 2019

@duailibe I tried to update from master, still conflicts

@fisker
Copy link
Member Author

fisker commented Jun 7, 2019

@brainkim I understand we should ES5, just too much work for this update, I temp use terse to ignore some error. I will check the build and locate the problem

@fisker fisker changed the title Rollup update [WIP] Rollup update Jun 7, 2019
@duailibe
Copy link
Collaborator

duailibe commented Jun 7, 2019

@fisker Did you update master?

@duailibe
Copy link
Collaborator

duailibe commented Jun 7, 2019

I wonder if we should use some kind of tool that automatically verifies ES5 compliance

@brodybits we already run tests with the final bundle in Node 4

@fisker
Copy link
Member Author

fisker commented Jun 7, 2019

@duailibe I saw you changed the sources, so external can't work, right?

Copy link
Member Author

@fisker fisker left a 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

@duailibe duailibe changed the title [WIP] Rollup update Rollup update Jun 7, 2019
@duailibe
Copy link
Collaborator

duailibe commented Jun 7, 2019

Iook like rollup-plugin-babel is not working as expected

Can you expand?

@fisker
Copy link
Member Author

fisker commented Jun 7, 2019

Iook like rollup-plugin-babel is not working as expected

?

As I checked, before uglify works, code should be ES5 by babel, but it's not, this is why uglify can't work

@fisker
Copy link
Member Author

fisker commented Jun 7, 2019

seems terser for webpack has some problem, maybe minify postcss later.

sorry, not well tested

@duailibe
Copy link
Collaborator

duailibe commented Jun 7, 2019

Yeah, we can't minify postcss-parser. I'm not sure why

@duailibe
Copy link
Collaborator

duailibe commented Jun 7, 2019

As I checked, before uglify works, code should be ES5 by babel, but it's not, this is why uglify can't work

I'm not sure why it didn't work, but the code is ES5 and is tested in Node 4.

@duailibe duailibe merged commit 99d4b86 into prettier:master Jun 7, 2019
@duailibe
Copy link
Collaborator

duailibe commented Jun 7, 2019

Thank you @fisker! Great job!

@fisker
Copy link
Member Author

fisker commented Jun 7, 2019

@duailibe the third-party code looks ugly in src/, also if later we want export another separate file, we have to change source again. I think we should keep my implemention

@fisker
Copy link
Member Author

fisker commented Jun 7, 2019

I'm not sure why it didn't work, but the code is ES5 and is tested in Node 4.

yes, this is why I start this pr, looks code is right, so I let you decide

@duailibe
Copy link
Collaborator

duailibe commented Jun 7, 2019

the third-party code looks ugly in src/

Why?

if later we want export another separate file

That's unlikely.. ideally we wouldn't even need the third-party.js.. the only reason we need it is to mock it later.

@fisker
Copy link
Member Author

fisker commented Jun 7, 2019

about ES5 check @brodybits mentioned before, I think there is a eslint-plugin-es may do the work

@thorn0
Copy link
Member

thorn0 commented Jun 7, 2019

Node 4 supports a lot of ES6 stuff. See https://node.green/

@duailibe
Copy link
Collaborator

duailibe commented Jun 7, 2019

@thorn0 You're right. It's not supposed to be ES5, it just needs to work in Node 4.

@thorn0
Copy link
Member

thorn0 commented Jun 7, 2019

@duailibe Which means we can't expect uglify to parse it.

@fisker
Copy link
Member Author

fisker commented Jun 7, 2019

@thorn0 @duailibe is standalone.js for browsers? we don't need support old browsers?

@duailibe
Copy link
Collaborator

duailibe commented Jun 7, 2019

@fisker Yes, it's for browser. We don't necessarily need to support old browsers.

We use whatever is matched by ">0.25%", "not ie 11", "not op_mini all" in browserslist.

@fisker
Copy link
Member Author

fisker commented Jun 7, 2019

why node 4? for node I think 8 is good enough

@duailibe
Copy link
Collaborator

duailibe commented Jun 7, 2019

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

@fisker
Copy link
Member Author

fisker commented Jun 7, 2019

@duailibe problem is babel seem not working right. need more test before release

@duailibe
Copy link
Collaborator

duailibe commented Jun 7, 2019

@fisker Ok, I didn't see any problem with babel. When you find out, let me know.

@fisker fisker deleted the rollup-update branch June 8, 2019 09:45
@lock lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Sep 6, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Sep 6, 2019
@lipis lipis added this to the 1.19 milestone Sep 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Update Rollup

5 participants