-
Notifications
You must be signed in to change notification settings - Fork 6
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
Align transpiling with babel & core-js future #1
Comments
Got a fork with the above changes here: https://github.com/rombrom/sapper-ie |
@rombrom nice work! I'll give your fork a go and then maybe it would be worth opening a pull request? |
Cool! Let me know if you run into any issues. |
@rombrom you should use |
@rombrom Tried it out in IE11 - and it works, it's nice :) Did you notice that this doesn't work on Edge any more though? I've specifically put in a library which uses rest-spread, which Edge doesn't support. The non-legacy version requires rest-spread to be processed by Babel in order to support Edge. |
I see. That's too bad... I expected core-js to pick it up as well but alas. Any idea why Edge specifically needs the rest-spread babel plugin? @zloirock, oh I didn't know. Combing through the core-js/babel info didn't make this apparent. I figured runtime-transforms would be separate from the preset-env polyfilling so it'd depend on your use-case. Do you know why only one of these options is necessary? @antony What do you want to do with this issue? I might continue experimenting with some setup later this week, but since your version is already a perfect working example I guess this issue is reaaaly low priority and more along the lines of "nice-to-have". |
Since each of whose options for different ways of polyfilling and they conflict together. |
@rombrom It doesn't work in Edge because you aren't babelifying the non-legacy build. Edge uses the non-legacy build, but doesn't support rest-spread. If yours provides smaller bundle-size, or faster compilation, and you can get it to work in Edge + IE11, then I'd happily take your changes, as it seems like it might be a cleaner solution. |
@zloirock thanks. I'm curious how these options differ concerning generated code. @antony I don't think taking the core-js approach will result in smaller bundles or faster compilation. The only foreseeable advantage is developer experience since we can polyfill features based on usage and browserslist targets. Having no core-js would mean a 'bring-your-own-polyfill/babel-transform' kind-of situation. Regardless, I've updated my fork. I'll try to take some time later this week to explore whether we can optimize build-time and bundle-size. |
@rombrom did you have any time to look at this? I'll try your updated fork soon and get back to you with results. |
@antony afraid not. I've found out last week with a new project that the default sapper template is actually pretty much OK in supporting IE11 and edge save a few polyfills (IIRC fetch and what's considered "default" on polyfill.io), so I haven't had the urgency (and time, sorry!) to look at the core-js options further and I don't know whether, if and/or when this becomes relevant again. I did do some more digging though as to why these polyfills are needed in the first place and that's because of this "issue" with shimport: Rich-Harris/shimport#4. Shimport loads the sapper JS in which I haven't encountered any issues regarding IE11/Edge support yet (haven't used object-spread yet, so...). Anyways, any polyfilling core-js would do happens after svelte's compilation. Adding core-js support in the babel-step would only make sense when you're a) using features you haven't already polyfilled (default; fetch) and/or b) when you're using features that svelte does not know how to handle. If we add in core-js like in my fork, the legacy bundles will always include all the polyfills needed by the browserslist, including the ones we already have with polyfill.io. Long story short, I have my doubts as to whether core-js should be included in the sapper-ie-template by default, since it impacts bundle size quite a lot. Maybe a different branch, or different sapper template repo (sapper-ie-es2019-template) might make more sense. Love to hear your thoughts on this! |
I've started leveraging core-js a lot more recently, and I'm pretty happy with the results. Once I get a minute I might try converting this repo to use core-js, I'm not really concerned about bundle size - I'm interested in only making the site work for IE11 users - I don't care how fast it is or how big it is or how nice it looks. If you use an obsolete browser you get an obsolete experience. |
It was already asked on Discord (by someone else and me) and StackOverflow, but is there a way to have an additional bundle, next to the client’s, server’s and service-worker’s? In this bundle, the polyfills needed by Another way could be putting them in a And also thank you @antony and @rombrom for making me understand what is needed to get the site to work in older browsers! |
First of let me say this is amazing work and saved me lots of hassle getting IE11 support to work.
After tinkering with the Rollup configuration I think it would be possible to simplify some stuff and make the transpiling-step a bit more efficient.
A few months ago I stumbled upon this doc in the core-js repo which outlines some of the work done on integrations between core-js and babel. Here the core-js contributors explain how the
@babel/polyfill
and@babel/runtime
packages will be deprecated in favour of handling polyfills and ES-proposals with the@babel/preset-env
and@babel/plugin-transform-runtime
with@babel/runtime-corejs3
packages. In some other projects I've successfully changed setups from using the (old) polyfills to core-js and I believe I've got it working for svelte as well.I noticed that in the current rollup setup the babel plugin is instantiated twice. I believe this could work with one pass—with a core-js integrated setup.
I've installed the following packages and versions:
And replaced lines 35-60 in the rollup-config.
As you can see the
@babel/plugin-proposal-object-rest-spread
is no longer needed. And after some testing it does not appear to negatively impact the builds. If you want I could fix up a pull-request for these changes somewhere in the coming days.The text was updated successfully, but these errors were encountered: