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

Transpile es5 #467

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

Brianzchen
Copy link
Contributor

Fixes #238
Add @babel/preset-env to rollup config which will transpile code down to es5

Does nothing to new Set() as that needs to be polyfilled and I haven't tested this on ie11 to see if that's necessary which could come in as a separate PR.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 11, 2020
@drarmstr drarmstr requested a review from mondaychen July 13, 2020 21:59
@mondaychen
Copy link
Contributor

Can you get rid of the changes in src?

@mondaychen
Copy link
Contributor

What's our goal of transpiling down to ES5? Do we want to make it work on IE 11 out-of-box?
If people need to add polyfill for Set, can we still say it works out-of box?

Another thing we should consider is which build targets we enable ES5 tranpiling. We have multiple build targets now. I think for ES and CommonJS builds, we expect people to use their bundlers, and it should be easy to add ES5 transpiling in their config file. For UMD builds, people can potentially use them directly in old browsers. For ES for browser build, people can only directly use it in modern browsers.

@Brianzchen
Copy link
Contributor Author

What's our goal of transpiling down to ES5? Do we want to make it work on IE 11 out-of-box?
If people need to add polyfill for Set, can we still say it works out-of box?

Another thing we should consider is which build targets we enable ES5 tranpiling. We have multiple build targets now. I think for ES and CommonJS builds, we expect people to use their bundlers, and it should be easy to add ES5 transpiling in their config file. For UMD builds, people can potentially use them directly in old browsers. For ES for browser build, people can only directly use it in modern browsers.

Yes the goal is to ship IE11 out of the box. I actually intended to make an incremental change but I've got my hands on a windows laptop from work. So I'll work on this further to make it runnable on IE11.

I think besides es the rest should be runnable out of the box in IE11. Since as I mentioned in one of the issues, typically a bundler doesn't transpile third party deps and only rolls them into the bundle with the setting, exclude: /node_modules/.

@JeremyRH
Copy link

@mondaychen @Brianzchen Webpack will use the "main" output by default but can also use the "module" output. The output es/recoil.js also needs to be transpiled to ES5.

@Brianzchen
Copy link
Contributor Author

@mondaychen @Brianzchen Webpack will use the "main" output by default but can also use the "module" output. The output es/recoil.js also needs to be transpiled to ES5.

That's right, I've just run some tests and need to work on my solution more. But webpack will pull the es module and will fail in edge and ie11. One of the issues is it can't parse the spread operator

@anajavi
Copy link

anajavi commented Jul 19, 2020

If people need to add polyfill for Set, can we still say it works out-of box?

React itself seems to require Set and Map polyfills: https://reactjs.org/docs/javascript-environment-requirements.html

@ajaybeniwal
Copy link

Is there any update on this will it be available for consumption in ie11

@Brianzchen
Copy link
Contributor Author

@ajaybeniwal Hey sorry, I've been busy with other projects so haven't done much work on this. My current solution doesn't actually work right now so I'd probably reevaluate the whole thing.

Given what I have on my plate, I'd work on this maybe in a few weeks, so if someone else can pick it up that'd be awesome.

@mondaychen
Copy link
Contributor

mondaychen commented Aug 18, 2020

@ajaybeniwal if you are using webpack or rollup, you can add @babel/preset-env to your own project and transpile recoil to IE11 compatible code.

@mondaychen
Copy link
Contributor

A Recoil team member expressed concerns on performance impact once we introduce ES5 transpiling. I hope we can built some simple performance benchmarking so we can feel comfortable shipping it.

@mondaychen
Copy link
Contributor

A note for @Brianzchen or someone who plan to pick this up: let's enable ES5 transpiling on UMD builds first. When we release 0.0.11, I will include a paragraph to explain how to use babel's preset-env for those who install it via NPM (I assume most people are using bundlers so it shouldn't be too hard to add that). In the future, we'll study the impact on performance and then decide if we do the same for all the builds.

Also, it would be very helpful if you can include the difference in output files (with and without ES5).

Thanks!

@Brianzchen
Copy link
Contributor Author

@mondaychen can you give some more context on how there's performance issue? Just that it's extremely rare for a web library to not ship with es5/ie11 compatibility out of the box and would make recoil quite non-standard in a way.

@mondaychen
Copy link
Contributor

Some concerns on performance were raised in issues like #228. When it comes ES5, I think the major concern is related to data structures we use like Map and Set. Currently we do a lot of copying on these data structures, which has nontrival impact on performance if you have a lot of atoms. Some ES5 polyfills might make it worse.
cc @davidmccabe

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build / infra CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ES6 code in production build breaks IE11
7 participants