Fix crash during server render in react 16.4.1. #13088

dustinsoftware · 2018-06-21T19:20:38Z

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.

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 · 2018-06-21T19:25:24Z

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.

dustinsoftware · 2018-06-21T19:37:05Z

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

Daniel15 · 2018-06-21T19:42:41Z

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.

not defined / not a function.

Daniel15 · 2018-06-21T19:46:27Z

@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?

gaearon · 2018-06-21T20:19:18Z

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.

gaearon · 2018-06-21T20:20:43Z

packages/react-scheduler/src/ReactScheduler.js

+const localSetTimeout =
+  typeof setTimeout === 'function'
+    ? setTimeout
+    : warning(


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

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

gaearon

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);

gaearon · 2018-06-21T23:50:57Z

packages/react-scheduler/src/ReactScheduler.js

@@ -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.',


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.

dustinsoftware · 2018-06-22T03:35:15Z

Ah, yes I did add that because of flow. I’ll push a fix.

…

On Thu, Jun 21, 2018 at 16:58, Dan Abramov ***@***.***> wrote: ***@***.**** requested changes on this pull request. 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); ------------------------------ In packages/react-scheduler/src/ReactScheduler.js <#13088 (comment)>: > @@ -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.', 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. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#13088 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA5hFuNBYwCxwupPa8to_KdpJF_awBcYks5t_DMVgaJpZM4UyoQq> .

dustinsoftware · 2018-06-22T18:55:41Z

I may be misunderstanding but it sounds like you prefer to have error message at all, so that’s what I pushed. If I misunderstood I can restore the warning inside an ”if dev mode” block so it is optimized out in production mode. On Thu, Jun 21, 2018 at 20:35, Dustin Masters <dustin@dustinsoftware.com> wrote:

…

Ah, yes I did add that because of flow. I’ll push a fix. On Thu, Jun 21, 2018 at 16:58, Dan Abramov ***@***.***> wrote: > ***@***.**** requested changes on this pull request. > > 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); > > ------------------------------ > > In packages/react-scheduler/src/ReactScheduler.js > <#13088 (comment)>: > > > @@ -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.', > > 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. > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#13088 (review)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AA5hFuNBYwCxwupPa8to_KdpJF_awBcYks5t_DMVgaJpZM4UyoQq> > . >

pull-bot · 2018-06-22T19:17:14Z

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

facebook-github-bot added the CLA Signed label Jun 21, 2018

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

dustinsoftware force-pushed the fix-ssr branch from c4b5885 to a2cd55a Compare June 21, 2018 19:21

Fix flow errors and log warning if setTimeout / clearTimeout are

d29667a

not defined / not a function.

gaearon reviewed Jun 21, 2018

View reviewed changes

Use invariant to assert setTimeout / clearTimeout are functions

98afa72

gaearon requested changes Jun 21, 2018

View reviewed changes

Remove use of invariant

ffd89fd

gaearon approved these changes Jun 22, 2018

View reviewed changes

Explain

96881ff

gaearon merged commit c35a1e7 into facebook:master Jun 22, 2018

gaearon mentioned this pull request Sep 5, 2018

Changelog for 16.5.0 #13571

Merged

dependencies bot mentioned this pull request Oct 1, 2018

Update react-dom in /modules/frontend from 16.3.0 to 16.5.2 project-accelerate/accelerate#277

Open

This was referenced Nov 1, 2018

Update react-dom in / from 16.2.0 to 16.6.0 leonardofaria/leozera-ui#253

Closed

Update react in / from 16.2.0 to 16.6.0 leonardofaria/leozera-ui#254

Closed

tnunes mentioned this pull request Nov 5, 2018

Fix crash during server render in React 16.6.0 #14103

Merged

This was referenced Dec 1, 2018

Update react-test-renderer in /modules/frontend from 16.3.0 to 16.6.3 project-accelerate/accelerate#312

Open

Update react in /modules/frontend from 16.3.0 to 16.6.3 project-accelerate/accelerate#313

Open

gaearon mentioned this pull request Mar 2, 2021

Clean up Scheduler forks #20915

Merged

This was referenced Sep 5, 2024

[Snyk] Fix for 104 vulnerabilities Omrisnyk/lock-walker#65

Open

[Snyk] Fix for 104 vulnerabilities Omrisnyk/lock-walker#66

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix crash during server render in react 16.4.1. #13088

Fix crash during server render in react 16.4.1. #13088

dustinsoftware commented Jun 21, 2018

gaearon commented Jun 21, 2018

dustinsoftware commented Jun 21, 2018

Daniel15 commented Jun 21, 2018 •

edited

Loading

Daniel15 commented Jun 21, 2018

gaearon commented Jun 21, 2018 •

edited

Loading

gaearon Jun 21, 2018

gaearon left a comment

gaearon Jun 21, 2018

dustinsoftware commented Jun 22, 2018 via email

dustinsoftware commented Jun 22, 2018 via email

pull-bot commented Jun 22, 2018

react-dom

react-art

react-scheduler

Fix crash during server render in react 16.4.1. #13088

Fix crash during server render in react 16.4.1. #13088

Conversation

dustinsoftware commented Jun 21, 2018

gaearon commented Jun 21, 2018

dustinsoftware commented Jun 21, 2018

Daniel15 commented Jun 21, 2018 • edited Loading

Daniel15 commented Jun 21, 2018

gaearon commented Jun 21, 2018 • edited Loading

gaearon Jun 21, 2018

Choose a reason for hiding this comment

gaearon left a comment

Choose a reason for hiding this comment

gaearon Jun 21, 2018

Choose a reason for hiding this comment

dustinsoftware commented Jun 22, 2018 via email

dustinsoftware commented Jun 22, 2018 via email

pull-bot commented Jun 22, 2018

react-dom

react-art

react-scheduler

Daniel15 commented Jun 21, 2018 •

edited

Loading

gaearon commented Jun 21, 2018 •

edited

Loading