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

Fix react-dom ReferenceError requestAnimationFrame in non-browser env (#13000) #13001

Conversation

sompylasar
Copy link
Contributor

The #12931 ( 79a740c ) broke the server-side rendering: in the fixtures/ssr the following error appeared from the server-side when localhost:3000 is requested:

ReferenceError: requestAnimationFrame is not defined
    at /__CENSORED__/react/build/node_modules/react-dom/cjs/react-dom.development.js:5232:34
    at Object.<anonymous> (/__CENSORED__/react/build/node_modules/react-dom/cjs/react-dom.development.js:17632:5)
    at Module._compile (module.js:624:30)
    at Module._extensions..js (module.js:635:10)
    at Object.require.extensions.(anonymous function) [as .js] (/__CENSORED__/react/fixtures/ssr/node_modules/babel-register/lib/node.js:152:7)
    at Module.load (module.js:545:32)
    at tryModuleLoad (module.js:508:12)
    at Function.Module._load (module.js:500:3)
    at Module.require (module.js:568:17)
    at require (internal/module.js:11:18)

The exception pointed to this line:

// We capture a local reference to any global, in case it gets polyfilled after
// this module is initially evaluated.
// We want to be using a consistent implementation.
const localRequestAnimationFrame = requestAnimationFrame;

Test plan

  1. In react repo root, yarn && yarn build.
  2. In fixtures/ssr, yarn && yarn start,
  3. In browser, go to http://localhost:3000.
  4. Observe the fixture page, not the exception message.

…facebook#13000)

The facebook#12931 ( facebook@79a740c ) broke the server-side rendering: in the `fixtures/ssr` the following error appeared from the server-side when `localhost:3000` is requested:
```
ReferenceError: requestAnimationFrame is not defined
    at /__CENSORED__/react/build/node_modules/react-dom/cjs/react-dom.development.js:5232:34
    at Object.<anonymous> (/__CENSORED__/react/build/node_modules/react-dom/cjs/react-dom.development.js:17632:5)
    at Module._compile (module.js:624:30)
    at Module._extensions..js (module.js:635:10)
    at Object.require.extensions.(anonymous function) [as .js] (/__CENSORED__/react/fixtures/ssr/node_modules/babel-register/lib/node.js:152:7)
    at Module.load (module.js:545:32)
    at tryModuleLoad (module.js:508:12)
    at Function.Module._load (module.js:500:3)
    at Module.require (module.js:568:17)
    at require (internal/module.js:11:18)
```

The exception pointed to this line:
```js
// We capture a local reference to any global, in case it gets polyfilled after
// this module is initially evaluated.
// We want to be using a consistent implementation.
const localRequestAnimationFrame = requestAnimationFrame;
```

**Test plan**

1. In `react` repo root, `yarn && yarn build`.
2. In `fixtures/ssr`, `yarn && yarn start`,
3. In browser, go to `http://localhost:3000`.
4. Observe the fixture page, not the exception message.
@gaearon
Copy link
Collaborator

gaearon commented Jun 8, 2018

Please run yarn flow dom and try to fix Flow.
There are probably multiple ways to do it.

I think it would make sense to remove the warning call from requestAnimationFrameForReact because we plan to get rid of that shim altogether later.

Then at the callsite you probably want to fire that warning if it doesn't exist, and substitute it by a shim in that case (e.g. that throws on call).

@pull-bot
Copy link

pull-bot commented Jun 8, 2018

ReactDOM: size: 🔺+0.1%, gzip: 🔺+0.1%

Details of bundled changes.

Comparing: d3e0a3a...aa90798

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.development.js +0.1% 0.0% 626.32 KB 626.65 KB 145.82 KB 145.87 KB UMD_DEV
react-dom.production.min.js 🔺+0.1% 🔺+0.1% 94.16 KB 94.26 KB 30.5 KB 30.53 KB UMD_PROD
react-dom.development.js +0.1% 0.0% 610.68 KB 611.01 KB 141.81 KB 141.87 KB NODE_DEV
react-dom.production.min.js 🔺+0.1% 🔺+0.1% 92.66 KB 92.76 KB 29.48 KB 29.51 KB NODE_PROD

react-art

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-art.development.js +0.1% 0.0% 407.9 KB 408.23 KB 91.04 KB 91.08 KB UMD_DEV
react-art.production.min.js 🔺+0.1% 🔺+0.1% 81.05 KB 81.15 KB 25 KB 25.03 KB UMD_PROD
react-art.development.js +0.1% +0.1% 333.75 KB 334.08 KB 72.13 KB 72.17 KB NODE_DEV
react-art.production.min.js 🔺+0.2% 🔺+0.2% 45.41 KB 45.51 KB 14.14 KB 14.17 KB NODE_PROD

react-scheduler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-scheduler.development.js +10.7% +9.1% 17.31 KB 19.17 KB 5.27 KB 5.74 KB UMD_DEV
react-scheduler.production.min.js 🔺+29.9% 🔺+28.9% 2.43 KB 3.16 KB 1.18 KB 1.53 KB UMD_PROD
react-scheduler.development.js +10.8% +9.2% 17.12 KB 18.98 KB 5.22 KB 5.7 KB NODE_DEV
react-scheduler.production.min.js 🔺+28.7% 🔺+29.9% 2.53 KB 3.25 KB 1.2 KB 1.55 KB NODE_PROD
ReactScheduler-dev.js +35.2% +33.1% 14.16 KB 19.15 KB 4.33 KB 5.77 KB FB_WWW_DEV
ReactScheduler-prod.js 🔺+17.3% 🔺+23.5% 7.21 KB 8.46 KB 1.88 KB 2.32 KB FB_WWW_PROD

Generated by 🚫 dangerJS

typeof requestAnimationFrameForReact === 'function'
? requestAnimationFrameForReact
: function(callback: Function) {
throw new Error(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please use invariant(false, message) here so the error message gets stripped out in prod.

? requestAnimationFrameForReact
: function(callback: Function) {
throw new Error(
'React depends on requestAnimationFrame, but this shim was called.',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's change the message to the same one as in the warning above.

@gaearon gaearon merged commit 188c425 into facebook:master Jun 8, 2018
@gaearon
Copy link
Collaborator

gaearon commented Jun 8, 2018

Thx

@flarnie
Copy link
Contributor

flarnie commented Jun 11, 2018

Thanks for fixing this! :)

@sompylasar
Copy link
Contributor Author

@gaearon @flarnie You're welcome. Unblocking #12063 one step at a time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants