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

requestAnimationFrame warnings on tests since React 16 #3199

Closed
barraponto opened this issue Sep 27, 2017 · 36 comments
Closed

requestAnimationFrame warnings on tests since React 16 #3199

barraponto opened this issue Sep 27, 2017 · 36 comments

Comments

@barraponto
Copy link

Is this a bug report?

Yes

Which terms did you search for in User Guide?

The issue is documented at https://facebook.github.io/react/docs/javascript-environment-requirements.html

Steps to Reproduce

  1. Start a project create-react-app project
  2. Try to run the template tests yarn test

Expected Behavior

I expected the testing to complete without warnings out-of-the-box (as it did pre-R16).

Actual Behavior

All tests pass, but prints message:

  console.error node_modules/fbjs/lib/warning.js:33
    Warning: React depends on requestAnimationFrame. Make sure that you load a polyfill in older browsers. http://fb.me/react-polyfills
@barraponto
Copy link
Author

I know React 16 needs that polyfill, my point is that create-react-app could provide it by default, taking the burden out of its users shoulders. For instance, it could depend on the raf package and change the test script in package.json to

{
  "scripts": {
    "test": "react-scripts test --env=jsdom --setupTestFrameworkScriptFile=raf/polyfill",
  },
}

@Timer
Copy link
Contributor

Timer commented Sep 27, 2017

Great point!
We know we need to start providing a Map and Set polyfill by default now, but we want to figure out a better story for including the polyfills (so they're not included for React 15 bundles, etc).

More to come on this soon, but happy to discuss further if you have some ideas!

@lukerollans
Copy link

lukerollans commented Sep 27, 2017

On the point of providing a Map and Set polyfill by default, my understanding is that babel-core loads said polyfill's automatically as per the babel-polyfill docs

Happy to be proven wrong, though.

@Timer
Copy link
Contributor

Timer commented Sep 27, 2017

@lukerollans I'm not sure what you're referring to, we do not depend on or include babel-polyfill by default.

Babel itself (babel-core) does not concern itself with polyfilling anything except some language features it uses itself (like Object.assign, via its _extends helper).


Polyfills wouldn't conflict with one another, so someone who uses babel-polyfill in their application wouldn't experience any bad side-effect (but you should never use the entirety of babel-polyfill or core-js).

@lukerollans
Copy link

I just double checked the source I linked to and I simply misread babel-node for babel-core, thanks for clearing it up!

@Timer
Copy link
Contributor

Timer commented Sep 27, 2017

Ah, no problem!

@Timer
Copy link
Contributor

Timer commented Sep 27, 2017

x-ref: #3200

@barraponto
Copy link
Author

@Timer i meant specifically for testing. Map and Set are already supported in Node 0.12+, so I think we can leave this issue just for CRA default test setup.

@Timer
Copy link
Contributor

Timer commented Sep 27, 2017

@barraponto correct! but we target IE 9 and Map and Set are not available in IE <11, so we'll just kill two birds with one stone! 😄

@holman
Copy link

holman commented Sep 27, 2017

fwiw, I know the current idea mentioned in various docs is to put this in src/setupTests.js:

global.requestAnimationFrame = function(callback) {
  setTimeout(callback, 0);
};

...but that doesn't seem to kill these warnings for me either after moving to 16.

@gaearon
Copy link
Contributor

gaearon commented Sep 27, 2017

We'll figure something out soon. Sorry about that. This was the biggest release we ever did, there's also a new website we're launching today, and between all that there's a few kinks we still need to iron out.

@jameslockwood
Copy link

jameslockwood commented Sep 28, 2017

@holman FYI I had the same problem RE shim not working. The gotcha is that polyfill/shim needs to be in it's own file, and loaded before any dependencies. See jestjs/jest#4545 (comment).

@holman
Copy link

holman commented Sep 28, 2017

Nice! Yeah that fixed it for me. Thanks for the heads-up @jameslockwood.

@unutoiul
Copy link

unutoiul commented Oct 5, 2017

I have following file setupTest.js:

import React from 'react'
import Enzyme, { shallow, render, mount } from 'enzyme';
import Adapter from 'enzyme-adapter-react-16';
import toJson from 'enzyme-to-json';

// React 16 Enzyme adapter
Enzyme.configure({ adapter: new Adapter() });

// Make Enzyme functions available in all test files without importing
global.shallow = shallow;
global.render = render;
global.mount = mount;
global.toJson = toJson;

// Fail tests on any warning
console.error = message => {
throw new Error(message);
};

global.requestAnimationFrame = function(callback) {
setTimeout(callback, 0);
};

but i still get the error:
Warning: React depends on requestAnimationFrame. Make sure that you load a polyfill in older browsers. http://fb.me/react-polyfills

I i keep only
global.requestAnimationFrame = function(callback) {
setTimeout(callback, 0);
};

works fine, if i put all of the above the error appers again...

any idea what is happening?

Thanks,
Lucas

@gaearon
Copy link
Contributor

gaearon commented Oct 5, 2017

It’s not an error. It is a warning. If you use Create React App, you can safely ignore it and nothing will happen.

We’ll release a fix that makes it disappear soon.

@gaearon
Copy link
Contributor

gaearon commented Oct 5, 2017

(The reason I think you’re getting it is because rAF is shimmed after Enzyme adapter is imported.)

@unutoiul
Copy link

unutoiul commented Oct 6, 2017

Thanks for your quick replay, I just realised I met you at a meeting organised by Sapient Nitro in London.

My solution was to create

src/tempPolyfills.js

const raf = global.requestAnimationFrame = (cb) => {
  setTimeout(cb, 0)
}

export default raf

src/setupTests.js

import raf from 'tempPolyfills'
import Enzyme, { shallow, render, mount } from 'enzyme';
import Adapter from 'enzyme-adapter-react-16';
import toJson from 'enzyme-to-json';

// React 16 Enzyme adapter
Enzyme.configure({ adapter: new Adapter() });

// Make Enzyme functions available in all test files without importing
global.shallow = shallow;
global.render = render;
global.mount = mount;
global.toJson = toJson;

// Fail tests on any warning
console.error = message => {
   throw new Error(message);
};

Thanks

@gaearon
Copy link
Contributor

gaearon commented Oct 8, 2017

Yep, that works! Again, sorry about this. I’ll look into it right after cutting 16.0.1 (which still needs a few more bugfixes to be ready).

@JamesTheHacker
Copy link

JamesTheHacker commented Oct 15, 2017

Adding --setupTestFrameworkScriptFile=raf/polyfill didn't work for me I was getting the error:

 Enzyme Internal Error: Enzyme expects an adapter to be configured, but found none. To
          configure an adapter, you should call `Enzyme.configure({ adapter: new Adapter() })`
          before using any of Enzyme's top level APIs, where `Adapter` is the adapter
          corresponding to the library currently being tested. For example:

I instead did this and everything works:

src/setupTest.js

import 'raf/polyfill';
import Enzyme, { shallow, render, mount } from 'enzyme';
import Adapter from 'enzyme-adapter-react-16';
import toJson from 'enzyme-to-json';

// React 16 Enzyme adapter
Enzyme.configure({ adapter: new Adapter() });

// Make Enzyme functions available in all test files without importing
global.shallow = shallow;
global.render = render;
global.mount = mount;
global.toJson = toJson;

// Fail tests on any warning
console.error = message => {
   throw new Error(message);
};

And adding the following to package.json: "test": "react-scripts test --env=jsdom --setupTestFrameworkScriptFile=./src/setupTests.js"

... basically the same as @unutoiul answer.

@unutoiul
Copy link

also the registerServiceWorker.js doesn't pass the eslint validation.

@gaearon
Copy link
Contributor

gaearon commented Oct 17, 2017

And adding the following to package.json: "test": "react-scripts test --env=jsdom --setupTestFrameworkScriptFile=./src/setupTests.js"

That sounds odd. You shouldn't need to do this, Create React App already runs the test setup file with this name automatically. Can you remove this and check if it still works?

javflores added a commit to javflores/photo-booth that referenced this issue Oct 17, 2017
@JamesTheHacker
Copy link

@gaearon ah yes you're right! I'm pretty sure I tried this before and it didn't work which is why I added the location as an argument.

@gaearon
Copy link
Contributor

gaearon commented Oct 17, 2017

Sweet. Again, I’m sorry this is still not resolved. I got sick with a stomach bug and have been slowly recovering this week. I’m mostly well now and expect to get to this in a couple of days.

@unutoiul
Copy link

unutoiul commented Oct 17, 2017

Hi Dan, what structure of the code you would recommend using to make RESTful API calls with react&redux?

@JamesTheHacker
Copy link

JamesTheHacker commented Oct 17, 2017

@unutoiul probably not the place for such questions but for making API calls, or dealing with side effects, you would want redux-thunk (Dan's project :p), or redux-sage. There's a lot of options tbh.

@dnaploszek
Copy link

Sadly @unutoiul solution does not work for me :(. The only difference is i'm using react-scripts-ts - TypeScript.
When i had only polyfill in setupTests.ts it worked, but after i added Enzyme and separated polyfill to a new file i cannot get this to work:

tempPolyfill.ts

const raf = global.requestAnimationFrame = (cb) => {
  setTimeout(cb, 0);
};

export default raf;

setupTests.ts

import raf from 'tempPolyfill';
import Enzyme, { shallow } from 'enzyme';
import Adapter from 'enzyme-adapter-react-16';

Enzyme.configure({adapter: new Adapter()});

// Make Enzyme functions available in all test files without importing
global.shallow = shallow;

Any clues?

@offero
Copy link

offero commented Oct 21, 2017

It’s not an error. It is a warning. If you use Create React App, you can safely ignore it and nothing will happen.

Except it exits with a non-zero exit code.

@gaearon
Copy link
Contributor

gaearon commented Oct 21, 2017

Can you provide a minimal example demonstrating this please? That sounds like a bug to me. It’s just a console.error() call so I don’t see why it would be a non-zero exit.

@gaearon
Copy link
Contributor

gaearon commented Oct 21, 2017

(If you configured your tests to fail on console.error then of course it would be a non-zero code, but configuring them as such would be your decision, not our default.)

@offero
Copy link

offero commented Oct 21, 2017

I have recently found the CI=true option and it's now exiting correctly. Thanks.

@danyalaytekin
Copy link

Looking forward to the fix for this, but no pressure. Hope you're feeling better @gaearon.

@gaearon
Copy link
Contributor

gaearon commented Oct 26, 2017

I think we'll fix this by updating to Jest version that already has a polyfill.
Or maybe we'll copy it over. I'll try to get it done today.

@gaearon
Copy link
Contributor

gaearon commented Oct 30, 2017

Should be fixed in [email protected].

https://github.com/facebookincubator/create-react-app/releases/tag/v1.0.15

@webmobiles
Copy link

it's possible when you don't want to work with older browser just turn off the warnings and errors on test ? or like android apps to specify the min. version supported ? or technically is not good idea? I think the idea to add polyfills when you dont' really need add more code to the build file ?
my build file have 1.6 Megabytes, i don't know why is so big

@Arelav
Copy link

Arelav commented Nov 16, 2017

@dnaploszek

Not worked for me with latest Create React App

This is my solution:

    "@types/enzyme": "^3.1.4",
    "@types/enzyme-adapter-react-16": "^1.0.1",
    "@types/jest": "^21.1.6",

setupTests.ts

import 'raf/polyfill';

// Temporary hack to suppress error
// https://github.com/facebookincubator/create-react-app/issues/3199
window.requestAnimationFrame = function (callback) {
  setTimeout(callback, 0);
  return 0;
};

import { configure } from 'enzyme';
import * as Adapter from 'enzyme-adapter-react-16';

configure({ adapter: new Adapter() });

@gaearon
Copy link
Contributor

gaearon commented Nov 17, 2017

This issue is starting to get noisy with potentially incorrect advice so I'm locking this. If you experience problems please file a new issue.

Note that as of [email protected] you shouldn't see this warning in tests. You would see it in older browsers (such as IE10) though. If you support those old browsers, you should add import 'raf/polyfill' as the first line of your src/index.js.

@facebook facebook locked and limited conversation to collaborators Nov 17, 2017
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