-
-
Notifications
You must be signed in to change notification settings - Fork 26.9k
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: Add option to build modern ES6 code #4964
base: main
Are you sure you want to change the base?
Conversation
This is really neat, @prichodko! Thanks for putting this together. I don't believe we're opposed to adding something like this, though I'm not sure if we'd want it behind a flag or on by default (assuming it works with backwards compatibility). Dan and I should have more time to focus on CRA in the coming weeks, so you'll likely hear much more from us soon. |
Great to hear that @Timer. 👍 Then, in upcoming days, I will write down my thoughts on how the final implementation could look like (so maybe other folks could chime in). |
f4f055a
to
8295dbd
Compare
@gaearon not considering anymore? |
@Timer ? |
Accident :-) |
the plugin is taken from here: facebook/create-react-app#4964 It works sometimes. The order of processing of configs is not fixed, and therfore at builds the es5 bundles are injected in place of es6 and vice versa
Another use-case to consider is dev vs. production builds. I'm gonna go nuts if I have to step through the regenerator runtime in a debugger again today. Except relatively rare cases of downlevel testing or debugging, developers will always be on a modern browser. There should be a way to configure CRA when in Dev mode to produce only ESnext (no ES5) so I won't have to pay the time and battery-life price of compiling ES5 dev builds that I'll never use. It's an interesting question of whether ES6-only should be the default for dev mode. Might be worth it if it saves a lot of build time in the default case. Feel free to ignore this feedback if compiling to ES5 and ESnext (as opposed to just ESnext) is really fast at build time. |
Any update to this PR? When will this feature be released? |
Hey, Let me start off by saying the bundle size diff would be bigger if libraries would ship a I also have a start to develop a feature like the following here My POC however is not ideal to be set up on a large scale that is why we should have to integrate it into some plugin/webpack. This being said, I support OP's notion to divide this into two seperate builds rather than making it into one huge multi compiled thing. This however makes it harder to make your .html file. Is there anything holding this feature back, I would love to help on it where I can. EDIT: in my POC I wrote a script for the shim and sliding in the bundles but apparently there's a neat plugin for this: https://github.com/firsttris/html-webpack-multi-build-plugin This way we can include the multiple bundles and the shim, so this could be included in the webpack config. I assume we just need to find how we would be able to add a build when --modern is added. |
@JoviDeCroock This is still something we'd like to have in the future. That webpack plugin looks like it could be useful. I think as @Timer said previously, we'd like to have this enabled by default and build both the modern and legacy bundles all the time and just let the browser decided which one to use. I'm not sure about the best way to proceed though. This PR is a bit out of date so it might be easier to start fresh. @prichodko Is this still something you're interested in working on? Perhaps the two of you could coordinate and work on it together. |
|
||
const ID = 'html-webpack-esmodules-plugin'; | ||
|
||
const safariFix = `(function(){var d=document;var c=d.createElement('script');if(!('noModule' in c)&&'onbeforeload' in c){var s=!1;d.addEventListener('beforeload',function(e){if(e.target===c){s=!0}else if(!e.target.hasAttribute('nomodule')||!s){return}e.preventDefault()},!0);c.type='module';c.src='.';d.head.appendChild(c);c.remove()}}())`; |
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.
hey but other browsers would download bundles twice or event thrice, isn't it?
issue with dicsussion philipwalton/webpack-esnext-boilerplate#1
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.
Haven't noticed this yet. I use it actively in this package: https://github.com/JoviDeCroock/webpack-module-nomodule-plugin
Have not fond any browsers yet that do this and it's running in a relatively big production app at this point. If you find any please let me know, I'll see what I can do about it
Tested with IE11 and did not download 2
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.
What about MS Edge? I tested my code and it loads ES6 bundle twice and ES5 bundle once.
Please, explain fast steps to run your code. I just wanted to test it out with my devices.
It would be very cool if code in this PR works as it should!
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.
well you can try out this repo: https://github.com/JoviDeCroock/POC-ModularLegacyBuild
Here's where I've done a full POC for the project I talked about in my first response (also uses ES6 libs in modern build)
Also maybe better to look at my recent PR since this is more up to date than this one.
#6590
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.
here is it, for JoviDeCroock/POC-ModularLegacyBuild
❌ it seems that IE11 on Win7 loads both bundles. Same for IE11 on Win10
❌MS Edge loads both bundles too (loads modern bundle twice)
✅ Safari 10.1 loads only modern bundle
I just faced same problem in my work process, and this is amazing that there is PR for CRA to load legacy&modern bundles.
Can I provide some friendly advice?
After my struggles (aka investigation process), I found that it's currently impossible to reliable load code trough only HTML code.
The only check which works on all problematic browsers is this:
'noModule' in HTMLScriptElement.prototype
That snippet returns true
on Safari 10.1. As for IE and MSEdge, here are some screenshots:
Also, I can be wrong. Feel free to correct me, as I don't have much experience with loading modern/legacy bundles at this very moment.
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.
As far as I see these quirks are not present in the new Microsoft browser and they are stepping back from supporting IE and the old edge so it's a tradeoff I'd take any day. Makes my app more performant but I closed the PR to CRA.
Every dev should consider it for their own user demographic
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.
aye, these are only presented in IE and MS Edge which would fade away very soon.
I just wanted to check solution out.
Totally agreed that every decision should be considered under usage and analysis
This PR is currently a proof of concept. If @Timer or @gaearon would agree to have this in CRA, there are some other improvements I would do before landing.
Background
All modern browsers support ES modules via script tag. That means that they support things like async/await, classes, arrow functions, fetch etc., which allows us to skip a lot of transpilation and polyfills and therefore build less verbose code, which should be faster to parse and run.
For more in depth explanation I recommend reading this great article by @philipwalton.
Implementation
It's based on my recent PR - #4852, which switches from unmaintained uglify-es to terser.
The implementation is inspired by an awesome work of the Vue team - link.
A new
--modern
flag is added to abuild
command.Code is built twice using webpack's multi-compiler. Firstly it's built using current configuration, secondly with
babel-preset-env
targets option set to esmodules.Then script tags in
index.html
are concatenated with correct attributes -type="module"
for modern code andnomodule
(+ safari fix) for legacy code.Improvements
For this PR I decided to go with webpack's
multi-compiler
, so I didn't have to modifybuild
command, but if we decide to ship it to CRA, I would separate it to two separate builds to improve overall DX (making in it clear what's being built, better handling of messages, diff between legacy and modern assets).Potentially extracting webpack plugin to separate module and making it less naive.
Other improvements could be to add support for rel=preload, but I think it's out of scope of this PR.
Results
There are some small improvements already when I tried that on the base template:
With a little custom configuration I was able to build Spectrum (cc @mxstbr):
While building Spectrum I found out that template literals are not minified, so size differences are not that huge.
Try it out
cd create-react-app/packages/react-scripts