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

Move polyfill control to user land #3922

Closed
wants to merge 2 commits into from

Conversation

andriijas
Copy link
Contributor

Rationale

In CRA 2.0 the user is given control to target browsers, which is a good thing. To take full advantage of browser targeting we should also put the control of polyfills in user land. Many types of apps dont need to support legacy browsers:

  • company internal dashboards
  • prototypes
  • mobile apps

Being able to control polyfills can make the bundle slimmer for certain apps which increases performence for road warriors using mobile connections for example.

CRA has always been about making it easy to get going with React, and CRA2 will of course build upon that and make sure its still easy to learn and explore React however I recognize a theme with the upcoming 2.0 release that extends beyond that which is - getting to know the output. Many issues and PR touches this theme such as vendor chunk splitting, cdn strategies not to mention bundle analyzation tools. Lets make CRA2 a release where users can be in the front seat of the output, getting to learn it and have options to tweak it depending on the product they are building.

Knowledge sharing

By putting the polyfills in user land we also have an opportunity for interested to learn more about polyfills - why they are needed and then. I tried to document the file as good as possible - Im open for reviews and improvements.

Opt-in by default

Opt-in by default makes it as easy to get going with React as in CRA1, you need to activly think about opt-ing out and the docs probably need to be clearer about the consequence when doing so. We don't want to repeat history (#2398)

Upgrading from CRA1

Here comes the biggest challenge I guess and here more implementation is needed. We need a way to support upgraders so that they dont unintentionally breaks their browser target chain. Im reaching out for guidelines how we can solve this.

Thoughts and feedback appreciated.

Closes #3343

PS: raf polyfill for testing intentionally removed as jsdom supports it and jest handles it.

@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!

@gaearon
Copy link
Contributor

gaearon commented Jan 26, 2018

I think this is reasonable although I'd like to hear more opinions.

One thing that I like about this is we could point users to polyfill.js examples for different target browsers (e.g. "how do I support IE9+?")

@viankakrisna
Copy link
Contributor

CMIW, but I thought babel 7 + browserlists makes the polyfills needed are included automatically so we don't need this file anymore?

@gaearon
Copy link
Contributor

gaearon commented Jan 26, 2018

No, Babel can't guess what features you use. It trims the things you definitely don't need from the default Babel polyfill, but that's only if you import it.

@Timer
Copy link
Contributor

Timer commented Jan 26, 2018

Babel's preset-env does have a usage mode which polyfills features you use in your code -- I'm not sure of how many edge cases it fails in, though.

I don't think moving polyfills to user-land is necessarily a good idea, React still supports IE 11 out of the box so most of these polyfills are still valid (and with automatic commons chunk coming the few kbs of polyfills wont be that big of a deal).

@viankakrisna
Copy link
Contributor

viankakrisna commented Jan 26, 2018

I feel like when we print the supported browser on the build script, the user would expect that it would work without adding any polyfills anymore (at least that's what I expect when I heard that we are using browserslist and babel-preset-env).

If we want to leave the user configuring their own polyfill, I think we need to update the message after the build that we only transform syntax feature. (maybe add another link to our docs)

@andriijas
Copy link
Contributor Author

We could have polyfills for supporting IE9 commented out already in the file.

Its also a nice container where developers can put their own polyfills of choice as well. babel-runtime or whatever.

Even if React supports IE11 we dont polyfill all features IE11 doesnt support. Ive made the misstake of using es6 features I thought was going to be transpiled/polyfilled automatically with CRA. Yikes, IE11 end of life is not until 2025. 😱

I agree if i configure target with browserlist that explicitly denies IE the build message will be confusing if polyfills are including.

@@ -1,5 +1,8 @@
import React from 'react';
import ReactDOM from 'react-dom';
// Include the default polyfills to enable modern javascript features
// in legacy browsers. Learn more: http://bit.ly/2Bt1Yjk
import './polyfills';
Copy link
Contributor

@viankakrisna viankakrisna Jan 31, 2018

Choose a reason for hiding this comment

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

this import should be placed before React, so the user doesn't have to move it if they want to include Map, Set, and RAF polyfill for React 16, it also matches current behavior.

@gaearon
Copy link
Contributor

gaearon commented Sep 27, 2018

I think we don't need this with new polyfilling strategy?

@Timer
Copy link
Contributor

Timer commented Sep 27, 2018

Nope, all polyfills are off by default now. Please contribute support for popular browsers (that need a default polyfill to work) to react-app-polyfill.

Thanks for all your efforts!

@Timer Timer closed this Sep 27, 2018
@andriijas andriijas deleted the polyfills branch October 12, 2018 08:54
@lock lock bot locked and limited conversation to collaborators Jan 18, 2019
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.

None yet

5 participants