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 crash during server render in react 16.4.1. #13088

Merged
merged 5 commits into from Jun 22, 2018

Conversation

@dustinsoftware
Copy link
Contributor

@dustinsoftware dustinsoftware commented Jun 21, 2018

This fixes a crash that only occurs in react-dom 16.4.1.

setTimeout and clearTimeout may not be available in some server-render environments (such as ChakraCore in React.NET), and loading ReactScheduler.js will cause a crash unless the existence of the variables are checked via a typeof comparison.

reactjs/React.NET#555

The crash did not occur in 16.4.0, and the change appears to have been introduced here: https://github.com/facebook/react/pull/12931/files#diff-bbebc3357e1fb99ab13ad796e04b69a6L47

I tested this by using yarn link and running it with a local copy of React.NET. I am unsure the best way to unit test this change, since assigning null to setTimeout causes an immediate crash within the Node REPL.

@dustinsoftware dustinsoftware changed the title This fixes a crash that only occurs in react-dom 16.4.1. Fix crash during server render in react 16.4.1. Jun 21, 2018
setTimeout and clearTimeout may not be available in some server-render environments (such as ChakraCore in React.NET), and loading ReactScheduler.js will cause a crash unless the existence of the variables are checked via a typeof comparison.

reactjs/React.NET#555

The crash did not occur in 16.4.0, and the change appears to have been introduced here: https://github.com/facebook/react/pull/12931/files#diff-bbebc3357e1fb99ab13ad796e04b69a6L47

I tested this by using yarn link and running it with a local copy of React.NET. I am unsure the best way to unit test this change, since assigning null to `setTimeout` causes an immediate crash within the Node REPL.
@gaearon
Copy link
Member

@gaearon gaearon commented Jun 21, 2018

Is there any way you can shim it on your side? We could take this but then it's highly likely we'll break it again. It's also plausible we'll start relying on e.g. Promise so if your environment is very unusual then you'll keep running into those issues.

Loading

@dustinsoftware
Copy link
Contributor Author

@dustinsoftware dustinsoftware commented Jun 21, 2018

Thanks for the fast response. I'll investigate adding a shim on React.NET for those functions and expect that it will break again in the future. Would you be open to accepting the patch anyway so we stop getting crashes in the meantime?

I opened a PR directly since this one took a similar approach: #13000

I'll look at fixing the flow errors, do you have any suggestions on what to do? Should we return a function that warns instead?

cc @Daniel15

Loading

@Daniel15
Copy link
Member

@Daniel15 Daniel15 commented Jun 21, 2018

Is there any way you can shim it on your side?

@gaearon - Relying on setTimeout and setInterval is going to break any non-browser rendering environment where the DOM/BOM is not available. Is it expected that ReactScheduler is only loaded in browser environments?

if your environment is very unusual then you'll keep running into those issues.

ReactJS.NET uses a pretty standard V8 or ChakraCore environment for server-side rendering.

Loading

@Daniel15
Copy link
Member

@Daniel15 Daniel15 commented Jun 21, 2018

@dustinsoftware In the meantime we can patch the version of React bundled with ReactJS.NET (although that means it'll still be broken for people that load their own version of React rather than using the bundled one). Either that or just shim setTimeout and setInterval, but that seems very hacky and may break other stuff. Since React seems to be pulling them into a variable, maybe shim it when React is being loaded, then unshim it once React has loaded?

Loading

@gaearon
Copy link
Member

@gaearon gaearon commented Jun 21, 2018

Relying on setTimeout and setInterval is going to break any non-browser rendering environment where the DOM/BOM is not available

We generally target Node environment with the server renderer. Node also has setTimeout.

I don’t think the server side renderer uses the scheduler. You’re likely seeing this issue because some component imports regular react-dom (e.g. for findDOMNode) which ends up pulling this initialization code in.

Loading

const localSetTimeout =
typeof setTimeout === 'function'
? setTimeout
: warning(
Copy link
Member

@gaearon gaearon Jun 21, 2018

Choose a reason for hiding this comment

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

This looks a bit misleading, as if we’re using its return value.

Let’s just keep undefined there explicitly and then warn later.

Loading

Copy link
Member

@gaearon gaearon left a comment

These are relatively hot paths and I'd like to avoid checking for something that for all intents and purposes will be true in the supported environments. (SSR doesn't currently call these functions.)

Let's remove the invariants? If Flow wants them, feel free to use any, e.g.

const localSetTimeout = typeof setTimeout !== 'undefined' ? setTimeout : (undefined: any);

Loading

@@ -104,6 +94,11 @@ if (!canUseDOM) {
callback: FrameCallbackType,
options?: {timeout: number},
): CallbackIdType {
invariant(
typeof localSetTimeout === 'function',
'localSetTimeout is not a function. Please load a polyfill that defines this function.',
Copy link
Member

@gaearon gaearon Jun 21, 2018

Choose a reason for hiding this comment

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

localSetTimeout is a variable name so it doesn't mean anything to somebody reading this. That said, I don't think we want an invariant at all.

Loading

@dustinsoftware
Copy link
Contributor Author

@dustinsoftware dustinsoftware commented Jun 22, 2018

Loading

@dustinsoftware
Copy link
Contributor Author

@dustinsoftware dustinsoftware commented Jun 22, 2018

Loading

@gaearon gaearon merged commit c35a1e7 into facebook:master Jun 22, 2018
1 check was pending
Loading
@pull-bot
Copy link

@pull-bot pull-bot commented Jun 22, 2018

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

Details of bundled changes.

Comparing: 5b3d17a...96881ff

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.development.js +0.1% +0.1% 626.73 KB 627.06 KB 146.5 KB 146.62 KB UMD_DEV
react-dom.production.min.js 🔺+0.1% 🔺+0.1% 95.3 KB 95.38 KB 30.82 KB 30.84 KB UMD_PROD
react-dom.development.js +0.1% +0.1% 622.86 KB 623.19 KB 145.33 KB 145.45 KB NODE_DEV
react-dom.production.min.js 🔺+0.1% 0.0% 95.29 KB 95.37 KB 30.35 KB 30.36 KB NODE_PROD
react-dom.profiling.min.js +0.1% 0.0% 96.26 KB 96.34 KB 30.68 KB 30.69 KB NODE_PROFILING

react-art

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-art.development.js +0.1% +0.1% 410.78 KB 411.11 KB 92.08 KB 92.19 KB UMD_DEV
react-art.production.min.js 🔺+0.1% 0.0% 82.49 KB 82.57 KB 25.42 KB 25.42 KB UMD_PROD
react-art.development.js +0.1% +0.2% 342.76 KB 343.09 KB 74.94 KB 75.06 KB NODE_DEV
react-art.production.min.js 🔺+0.2% 🔺+0.1% 47.47 KB 47.55 KB 14.79 KB 14.8 KB NODE_PROD

react-scheduler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-scheduler.development.js +2.2% +2.4% 15.1 KB 15.43 KB 4.73 KB 4.85 KB UMD_DEV
react-scheduler.production.min.js 🔺+3.1% 🔺+1.3% 2.43 KB 2.51 KB 1.18 KB 1.2 KB UMD_PROD
react-scheduler.development.js +2.2% +2.4% 14.91 KB 15.24 KB 4.69 KB 4.8 KB NODE_DEV
react-scheduler.production.min.js 🔺+3.3% 🔺+1.4% 2.34 KB 2.42 KB 1.13 KB 1.14 KB NODE_PROD
ReactScheduler-dev.js +2.4% +2.7% 13.67 KB 14 KB 4.21 KB 4.33 KB FB_WWW_DEV
ReactScheduler-prod.js 🔺+1.3% 🔺+0.8% 6.91 KB 7 KB 1.8 KB 1.82 KB FB_WWW_PROD

Generated by 🚫 dangerJS

Loading

TejasQ pushed a commit to TejasQ/react that referenced this issue Aug 26, 2018
* Fix crash during server render.

setTimeout and clearTimeout may not be available in some server-render environments (such as ChakraCore in React.NET), and loading ReactScheduler.js will cause a crash unless the existence of the variables are checked via a typeof comparison.

reactjs/React.NET#555

The crash did not occur in 16.4.0, and the change appears to have been introduced here: https://github.com/facebook/react/pull/12931/files#diff-bbebc3357e1fb99ab13ad796e04b69a6L47

I tested this by using yarn link and running it with a local copy of React.NET. I am unsure the best way to unit test this change, since assigning null to `setTimeout` causes an immediate crash within the Node REPL.

* Fix flow errors and log warning if setTimeout / clearTimeout are
not defined / not a function.

* Use invariant to assert setTimeout / clearTimeout are functions

* Remove use of invariant

* Explain
@gaearon gaearon mentioned this pull request Sep 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants