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

Drop Jest polyfill for requestAnimationFrame #4129

Closed
kentcdodds opened this issue Mar 9, 2018 · 6 comments
Closed

Drop Jest polyfill for requestAnimationFrame #4129

kentcdodds opened this issue Mar 9, 2018 · 6 comments
Milestone

Comments

@kentcdodds
Copy link
Contributor

Sorry, the template didn't really apply here.

I just noticed that in master, react-scripts depends on jest@22.1.2. As of jest@22.0.0, jest-environment-jsdom upgraded to jsdom@11.4 which includes built-in support for requestAnimationFrame.

I'm fairly confident that all that needs to happen here is to simply remove these lines:

// In tests, polyfill requestAnimationFrame since jsdom doesn't provide it yet.
// We don't polyfill it in the browser--this is user's responsibility.
if (process.env.NODE_ENV === 'test') {
require('raf').polyfill(global);
}

Oh, and remove the dependency:

Oh, and while we're at it, if create-react-app is supporting node@>=6 then we can probably get rid of the Object.assign polyfill (and the object-assign dep) as well as the Promise polyfill (and the promise dep).

That should leave the file pretty bare. Something like this I think:

// @remove-on-eject-begin
/**
 * Copyright (c) 2015-present, Facebook, Inc.
 *
 * This source code is licensed under the MIT license found in the
 * LICENSE file in the root directory of this source tree.
 */
// @remove-on-eject-end
'use strict';

// fetch() polyfill for making API calls.
require('whatwg-fetch');
@Timer Timer added this to the 2.0.0 milestone Mar 9, 2018
@Timer
Copy link
Contributor

Timer commented Mar 9, 2018

I believe you meant next 🤔. But thanks for pointing this out!

As for the polyfills, react-scripts@x will still support IE11 by default so I think it makes sense to keep the Object.assign and Promise polyfills.

@kentcdodds
Copy link
Contributor Author

kentcdodds commented Mar 9, 2018

Right but this is in the testing environment which will be node 6 or greater as indicated by the engines config in the package.json I linked to 😉

@Timer
Copy link
Contributor

Timer commented Mar 9, 2018

polyfills.js is required in the web builds and by the test environment.

entry: [require.resolve('./polyfills'), paths.appIndexJs],

@kentcdodds
Copy link
Contributor Author

Ah, that makes sense. Whelp, in that case we could either:

  1. Just over-polyfill in the tests (NBD)
  2. Separate out polyfills in tests and browser 🤷‍♂️

I don't really care either way personally :)

@andriijas
Copy link
Contributor

Personally Id like the polyfills moved to user land. I know it comes with learning curve but now that we have browserlist targeting in next I think it makes sense to use a service like polyfill.io and improve docs on how users can tweak polyfilling. Which polyfills to use depending on the environments they are targeting etc.

@Timer
Copy link
Contributor

Timer commented Sep 26, 2018

Resolved for next beta

@Timer Timer closed this as completed Sep 26, 2018
@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

No branches or pull requests

3 participants