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

WIP: differential serving allow modern ES2015 and legacy ES5 build #6590

Closed
wants to merge 15 commits into from

Conversation

JoviDeCroock
Copy link

@JoviDeCroock JoviDeCroock commented Mar 7, 2019

  • provide dynamic babel config
  • provide dynamic webpack config
  • Add to react-dev-utils
  • opt-in with --modern
  • optimize babel config (lose some polyfills)
  • test file size differences
  • write documentation about --modern

For now this is a WIP since I did not know if we wanted to opt-in by default or opt-out by default.

So when running the production yarn build at this point it will build your files with .modern and normal. This will be served when opening in chrome you will see the .modern in your network tab and when opening on IE11 you'll see the normal ones.

Also I have personally been advocating heavily for .modern bundles in libraries, this would impact file size differences even more, since now every library is shipped as ES2015 by default (I don't think this is feasible for now).

big thanks to @prichodko in this PR which has heavily influenced the webpack plugin.

Own POC with polyfills for legacy and none for modern

I would like to spark some discussion about the unfinished points before I continue if this is possible.

Related: https://github.com/facebook/create-react-app/projects/5#card-15632785

EDIT: I would like to know what babel-preset-react-app/dependencies, from what I understand you transpile your deps down a bit more than usual. So in modern config we would just not include this?

If anyone has doubts about this feature, I recently made a big post about browser support etc here

NOTE: this needs a new publish of react-dev-utils

@facebook-github-bot
Copy link

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@iansu iansu self-assigned this Mar 7, 2019
@ianschmitz
Copy link
Contributor

I'm not sure a dual build is what we'll end up wanting. I know we've talked about doing dual build in the past, but i really think we should re-evaluate using browserslist in @babel/preset-env to accomplish this. We already use this for PostCSS.

I've removed the ignoreBrowsersList and targets on @babel/preset-env and had great success on a number of projects.

@ianschmitz
Copy link
Contributor

Related: #6198

@JoviDeCroock
Copy link
Author

Making your browserslist configurable seems pretty unrelated to this feature, this feature enables the developer to treat the evergreen users better while not dropping support for nevergreen.

@ianschmitz
Copy link
Contributor

That's fair @JoviDeCroock. I think these features can live in tandem. The Vue team took the approach of respecting browserslist by default and providing a modern mode that emits the dual build like you're working on here. It seems to be working well for them: https://cli.vuejs.org/guide/browser-compatibility.html#polyfills-when-building-as-library-or-web-components

@JoviDeCroock
Copy link
Author

Yes, I really am all for what you said about the browserslist but I think this is also a big oppurtunity for projects willing to support older browsers and providing that little bit extra for their other customers.

That being said, I would love to move forward with this feature whenever someone has time to talk about the points made in the start of this pr

@JoviDeCroock
Copy link
Author

Hey could I please get some answers on these 3 points before I can continue (since the first fails my tests)

  Is this needed in development (?) IMO, not really
  allow de-opt through cli, with a --legacy flag?```

@ianschmitz
Copy link
Contributor

Is this needed in development

No, i don't think this is needed in development.

allow de-opt through cli, with a --legacy flag?

I think exposing this via a --modern flag is a good starting point and allows us to release this in a minor release. We can consider making modern the default in a future major.

@JoviDeCroock
Copy link
Author

Allright and how do I make the tests for eject work? Since this probably requires a utils release?

@ianschmitz ianschmitz added this to the 3.x milestone Mar 14, 2019
Copy link
Contributor

@ianschmitz ianschmitz left a comment

Choose a reason for hiding this comment

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

We'll want to update react-dev-utils/README.md to document the new plugin.

@JoviDeCroock
Copy link
Author

JoviDeCroock commented Mar 15, 2019

Okay so the --modern flag works.

Now all we have to do is optimise the config for modern browsers since this:

File sizes after gzip:

  36.4 KB  build\static\js\2.073e8f0b.modern.chunk.js
  36.4 KB  build\static\js\2.71f54e92.chunk.js
  770 B    build\static\js\runtime~main.53198464.modern.js
  762 B    build\static\js\runtime~main.a8a9905a.js
  598 B    build\static\js\main.511e72ba.chunk.js
  598 B    build\static\js\main.d551d14c.modern.chunk.js

In all honest does not make it worth it ^^ note that these numbers were different before merging master in.

In comparison to what is stated here: #4964

In the previous PR they removed transform-runtime and some other plugins. I do not know how far we are willing to go with this since I'm not aware of the implications you have been through that you had to add these. I'll read up on the issues in the comments and try to make a calculated decision.

I'm starting to get the feeling that something just overrides the babelconfig I'm making... Since the normal (not .modern) outputs are 8 bytes smaller...

@ianschmitz
Copy link
Contributor

Hmm this may be of use: https://github.com/vuejs/vue-cli/blob/93f57ac4a90e1d609183d00b77470c2e77bcaf63/packages/%40vue/babel-preset-app/index.js.

Doesn't look like they're doing anything too special for modern?

@ianschmitz
Copy link
Contributor

ianschmitz commented Mar 16, 2019

Maybe double check the outputted .js to make sure it looks as we expect. For example if you declare a class or an async function(), I would imagine that they would be left as is in the .modern.js bundles.

@JoviDeCroock
Copy link
Author

When I implement the starting component as a class it shows a difference, the initial bytes more on modern do not make sense though. In vue cli they remove corejs in general for polyfilling. This is one of the differences being made here.

@JoviDeCroock
Copy link
Author

useBuiltIns: 'usage' as used in vue makes the diff more significant

  36.79 KB  build\static\js\2.8f71a45e.chunk.js
  36.4 KB   build\static\js\2.0ef311fe.modern.chunk.js
  770 B     build\static\js\runtime~main.53198464.modern.js
  762 B     build\static\js\runtime~main.a8a9905a.js
  710 B     build\static\js\main.a89b9cde.chunk.js
  622 B     build\static\js\main.86f918ca.modern.chunk.js

@ianschmitz
Copy link
Contributor

useBuiltIns: 'usage' as used in vue makes the diff more significant

Interesting! I wonder why that matters. You're not importing @babel/polyfill or core-js or anything in your test are you?

We're probably not gonna see big differences in the default CRA template. It would be great if we could run these tests on a larger project instead.

@JoviDeCroock
Copy link
Author

JoviDeCroock commented Mar 18, 2019

I’ll see if I can find any bigger applications, if you know any other things like for example not including object rest spread? Because modern ones have it?

Testing with yarn link feels like a pandora’s box of unresolved dependencies :/ it just keeps on saying it can’t find x or y....

Specifically typescript and related... got it fixed, diffs are so minimal as opposed to the 20% of previous pr/vue cli.

The best result I've gotten so far is 4KB off which in % was about 3, ofcourse libraries can't be transpiled "up" to ES6 and still make for a big part of the overhead but I still think it's weird to see our results making this small of a diff compared to the prev PR.

Differences on my own POC are also a lot bigger, going to try and find some time to debug this issue soon.

I have published the plugin: this one is a little more flexible than the one provided in the cra code https://www.npmjs.com/package/webpack-module-nomodule-plugin

@lock lock bot locked and limited conversation to collaborators Apr 16, 2019
@JoviDeCroock JoviDeCroock deleted the feat/modern-build branch November 15, 2020 18:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants