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

[Fiber] Check for requestAnimationFrame throws when react-dom is required on the server #9102

Closed
ide opened this issue Mar 3, 2017 · 5 comments
Closed

Comments

@ide
Copy link
Contributor

@ide ide commented Mar 3, 2017

Do you want to request a feature or report a bug?
Bug-ish

What is the current behavior?
When doing server-side rendering with React, requiring react-dom (which transitively requires ReactDOMFrameScheduling.js) throws an exception because rAF is not defined.

Invariant Violation: React depends on requestAnimationFrame. Make sure that you load a polyfill in older browsers.
    at invariant (app/node_modules/fbjs/lib/invariant.js:44:15)
    at Object.<anonymous> (app/node_modules/react-dom/lib/ReactDOMFrameScheduling.js:30:3)

This can happen if you have a universal component that has top-level imports of client-side libraries, like react-router-scroll, that require react-dom instead of react-dom/server.

What is the expected behavior?
I would expect not to get this error unless requestAnimationFrame were actually called. Ex: lazily check for rAF and define rIC.

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?
React 16.0.0-alpha.3 on Node 7.7.1. This started happening in alpha 3, which shipped and enabled Fiber.

@ide
Copy link
Contributor Author

@ide ide commented Mar 3, 2017

A workaround that achieves my expected behavior is for each consumer of React to define a little polyfill in a file that's imported before the others but I expect many people to run into this issue. This is what it looks like:

global.window = global;
window.addEventListener = () => {};
window.requestAnimationFrame = () => {
  throw new Error('requestAnimationFrame is not supported in Node');
};
@gaearon
Copy link
Member

@gaearon gaearon commented Mar 3, 2017

We could fix this by lazily initializing rAF polyfill, but now that I think of it, it seems unfortunate that merely importing findDOMNode (which is likely what components do) runs the whole client reconciler init code.

moroshko added a commit to moroshko/react-autosuggest that referenced this issue Mar 22, 2017
Can't run tests due to facebook/react#9102
@ManasJayanth
Copy link
Contributor

@ManasJayanth ManasJayanth commented Apr 9, 2017

@gaearon Shouldn't it have been caught in the unit test

Also, more importantly, isn't requiring react-dom and running findDOMNode on server an anti-pattern, and that components must avoid them in hooks that run on the server?

@gaearon
Copy link
Member

@gaearon gaearon commented Apr 9, 2017

Also, more importantly, isn't requiring react-dom and running findDOMNode on server an anti-pattern, and that components must avoid them in hooks that run on the server?

You shouldn't run it on the server, but importing it is fine. Otherwise how would you write a component that uses findDOMNode on the client but still works on the server?

Shouldn't it have been caught in the unit test

No, the test doesn’t catch this because we set up a global rAF polyfill as part of our testing environment.

@ManasJayanth
Copy link
Contributor

@ManasJayanth ManasJayanth commented Apr 10, 2017

Edit: The PR is just for illustration. Just ran the unit tests. No linting and prettier run yet.

Opened a PR to put down the idea in code.

I moved the check for rAF inside the ReactFiberScheduler

But now, I lost the ability to throw if the rAF is not polyfilled. Is this the right place for the lazy check you mentioned?

ManasJayanth added a commit to ManasJayanth/react that referenced this issue Apr 18, 2017
ManasJayanth added a commit to ManasJayanth/react that referenced this issue Apr 18, 2017
ManasJayanth added a commit to ManasJayanth/react that referenced this issue Apr 20, 2017
ManasJayanth added a commit to ManasJayanth/react that referenced this issue Apr 20, 2017
ManasJayanth added a commit to ManasJayanth/react that referenced this issue Apr 21, 2017
ManasJayanth added a commit to ManasJayanth/react that referenced this issue Apr 25, 2017
ManasJayanth added a commit to ManasJayanth/react that referenced this issue Apr 25, 2017
ManasJayanth added a commit to ManasJayanth/react that referenced this issue Apr 26, 2017
ManasJayanth added a commit to ManasJayanth/react that referenced this issue Apr 26, 2017
@gaearon gaearon closed this in 182642b Apr 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants