Fiber ReactDOM shouldn't throw on import in Node environment if it's unused #9389

ManasJayanth · 2017-04-10T02:36:46Z

Patch for #9102

ManasJayanth · 2017-04-18T04:49:34Z

@gaearon Hi, PRs awaiting review. Please have a look.

gaearon · 2017-04-18T14:59:21Z

I don't think this is the right solution. The fact that DOM implementation might not have a way to schedule the animation seems like an implementation detail of the DOM renderer. So at the very least it shouldn't change anything in ReactFiberScheduler.

ManasJayanth · 2017-04-18T15:09:01Z

From your comment here

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.

I'm a little confused: to lazy check for the rAF polyfill or to look at DOM implementation itself (which seems like a major overhaul).

gaearon · 2017-04-18T15:36:58Z

We can check ExecutionEnvironment.canUseDOM. If it's false maybe we could provide a "fake" polyfill for requestAnimationFrame that just calls setTimeout.

ManasJayanth · 2017-04-20T15:44:54Z

src/renderers/shared/ReactDOMFrameScheduling.js


 // TODO: There's no way to cancel these, because Fiber doesn't atm.
 let rAF: (callback: (time: number) => void) => number;
 let rIC: (callback: (deadline: Deadline) => void) => number;
+
+if (!ExecutionEnvironment.canUseDOM) {


@gaearon /cc

There's a window.addEventListener on L92 too. Use the same trick there?

Or fake polyfill requestIdleCallback itself?

On L92 there a window.addEventListener.

Fake polyfill requestIdleCallback too?

gaearon · 2017-04-20T16:26:07Z

src/renderers/shared/ReactDOMFrameScheduling.js


 // TODO: There's no way to cancel these, because Fiber doesn't atm.
 let rAF: (callback: (time: number) => void) => number;
 let rIC: (callback: (deadline: Deadline) => void) => number;
+
+if (!ExecutionEnvironment.canUseDOM) {
+  global.requestAnimationFrame = function(


We should not put anything on global ourselves.

gaearon · 2017-04-20T16:28:08Z

src/renderers/shared/ReactDOMFrameScheduling.js


 // TODO: There's no way to cancel these, because Fiber doesn't atm.
 let rAF: (callback: (time: number) => void) => number;
 let rIC: (callback: (deadline: Deadline) => void) => number;
+
+if (!ExecutionEnvironment.canUseDOM) {
+  global.requestAnimationFrame = function(


Take a look at the code below. We export rAF and rIC from this file. So we should just assign those variables instead of globals.

But that wont prevent the codepath accessing requestAnimationFrame (and turns out requestIdleCallback too. L101 is breaking on the server too.

So wrap the entire polyfilling code in else block of the if (!ExecutionEnvironment.canUseDOM) {?

I imagine something like

if (!ExecutionEnvironment.canUseDOM) { } else if (typeof requestAnimationFrame !== 'function') { } else if (typeof requestIdleCallback !== 'function') { } else { }

This way you won’t get into that condition at all if DOM is unavailable.

gaearon

Please remove the global assignment.

ManasJayanth · 2017-04-21T00:20:38Z

@gaearon Requested changes have been made.

gaearon · 2017-04-21T08:46:26Z

Can we also add a test for this? "Can import findDOMNode in Node environment." The test would have a try-finally block in which it deletes global.requestAnimationFrame() and global.requestIdleCallback(), calls jest.resetModules(), and then tries to require('ReactDOM'). In the finally block it would restore the deleted globals.

ManasJayanth · 2017-04-21T11:15:15Z

I'm not entirely familiar with the testing stack yet. This led to the following:

I was unable to require the Fiber version of react dom. Had to set ReactDOMFeatureFlags.useFiber to true. Same with ExecutionEnvironment.canUseDOM has to set it to false. Is it okay to toggle them this way?
delete didn't work some reason. Had to set requestAnimationFrame and requestIdleCallback to undefined

Also, I added to 2 identical tests. One with the try/finally block as you suggested. Other, using beforeEach and afterEach as seen in other tests. Feel free to let me know which one should be removed.

gaearon · 2017-04-21T11:20:26Z

Is it okay to toggle them this way?

No, you don’t need to do this. I think you should move them to ReactDOMFiber-test behind this check. To actually run them, you need to either run REACT_DOM_JEST_USE_FIBER=true npm test <testfilename>, or run ./scripts/fiber/record-tests (which will run all tests with Fiber feature flag and record the passing and failing tests).

delete didn't work some reason. Had to set requestAnimationFrame and requestIdleCallback to undefined

That sounds fine.

Feel free to let me know which one should be removed.

I’d like to keep deletions like this confined to a single test case. So let’s use the try / finally.

gaearon · 2017-04-21T11:20:51Z

src/renderers/shared/__tests__/ReactDOMFrameScheduling-test.js

+describe('ReactDOMFrameScheduling (try/finally)', () => {
+  it('can import findDOMNode in Node environment.', () => {
+    var ReactDOM, ReactDOMFeatureFlags, ExecutionEnvironment;
+    var _requestAnimationFrame = global.requestAnimationFrame;


Naming nitpick: let's call this previousRAF (and previousRIC).

gaearon · 2017-04-21T11:21:52Z

Also let's add tests both for:

the new behavior
an invariant when importing ReactDOM on the client if canUseDOM is still true but rAF and rIC are missing

ManasJayanth · 2017-04-21T14:11:37Z

Ran the test on master. Reseting module cache leads to two failing tests from disableNewFeatures

ManasJayanth · 2017-04-21T14:14:37Z

FAIL  src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js
● ReactDOMFiber › can import findDOMNode in Node environment.

Invariant Violation: React depends on requestAnimationFrame. Make sure that you load a polyfill in older browsers.

at invariant (node_modules/fbjs/lib/invariant.js:44:15)
at Object.<anonymous> (src/renderers/shared/ReactDOMFrameScheduling.js:49:3)
at Object.<anonymous> (src/renderers/dom/fiber/ReactDOMFiber.js:24:31)
at Object.<anonymous> (src/renderers/dom/__mocks__/ReactDOM.js:17:1)
at Object.<anonymous> (src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js:92:9)

● disableNewFiberFeatures › throws if non-element passed to top-level render

expect(function).toThrow(string)

Expected the function to throw an error matching:
"render(): Invalid component element."
But it didn't throw anything.

at Object.<anonymous> (src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js:1043:68)

● disableNewFiberFeatures › throws if something other than false, null, or an element is returned from render

expect(function).toThrow(regexp)

Expected the function to throw an error matching:
/You may have returned undefined/
But it didn't throw anything.

at Object.<anonymous> (src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js:1062:157)

gaearon · 2017-04-21T14:30:23Z

src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js

+        global.requestAnimationFrame = undefined;
+        global.requestIdleCallback = undefined;
+        jest.resetModules();
+        require('ReactDOM');


This is actually expected to throw since it determines you're in DOM mode. You need to keep this test (but change it to assert that it throws), and add another test that fools canUseDOM somehow to return false (look for existing tests that might do something similar—maybe it's enough to delete window.document or something like that).

What about disableNewFiberFeatures tests that are failing?

Maybe they’re failing because they don’t expect a new resetModules in an arbitrary test? Perhaps it’s better to keep these two tests in a different file then, but keep them wrapped in the useFiber check.

Back next to ReactDOMFrameScheduling?

ManasJayanth · 2017-04-23T12:41:23Z

@gaearon Tests have been added.

Due to the useFiber check, when not run in use fiber mode, test runner complains 'There should be atleast one test'.
Running prettier leads to a lot of files being modified, adding to a lot of noise on this PR. Why is this? Looks like its the recent prettier upgrade. Why do I have to run it and commit again? Did I miss something in the workflow?

ManasJayanth · 2017-04-25T11:35:58Z

@gaearon ^^

gaearon · 2017-04-25T13:02:51Z

Due to the useFiber check, when not run in use fiber mode, test runner complains 'There should be atleast one test'.

Let's write

const describeFiber = ReactDOMFeatureFlags.useFiber ? describe : xdescribe;
describeFiber('ReactDOMFrameScheduling', () => {
  // ...
});

This way it would get skipped in Stack completely. I think this should work (ReactPerf test does the same, but the other way around).

Why do I have to run it and commit again? Did I miss something in the workflow?

You probably want to do something like

git fetch orign
git rebase origin/master

ManasJayanth · 2017-04-25T15:39:13Z

git fetch orign
git rebase origin/master

I guess you meant upstream. It still didn't work. When I ran prettier again locally, it prettifying the files all over again

Also, I had to write a dummy it() to make sure jest actually skipped it.

gaearon · 2017-04-25T15:40:26Z

You don't need to have an it there—I'm suggesting that you can now remove the if (ReactDOMFeatureFlags.useFiber) { check.

ManasJayanth · 2017-04-25T15:41:24Z

Updating..

ManasJayanth · 2017-04-25T15:41:35Z

And the prettier issue?

gaearon · 2017-04-25T15:46:00Z

Did you run yarn after rebasing?

ManasJayanth · 2017-04-25T15:48:31Z

I've been using the npm commands as mentioned in the Contributing docs. Running yarn now..

ManasJayanth · 2017-04-25T15:49:51Z

I haven't had this issue before

gaearon · 2017-04-25T15:51:13Z

Since you last checked the source out, Prettier has updated. You are running an older version of Prettier so this is why it formats differently. Running yarn will give you the same version as specified in the package.json (and the lockfile).

ManasJayanth · 2017-04-25T16:03:38Z

I vaguely remember running npm install after seeing the prettier update commit. Turns out I hadn't. Thank you so much.

ManasJayanth · 2017-04-25T17:08:32Z

@gaearon All good? Squash the two commits into one?

gaearon · 2017-04-25T19:31:15Z

src/renderers/shared/__tests__/ReactDOMFrameScheduling-test.js

+    jest.resetModules();
+    expect(() => {
+      require('ReactDOM');
+    }).toThrow();


Can you test for specific error message here please?

Sure. My bad actually. I have pushed the changes. Please have a look.

gaearon · 2017-04-27T12:13:44Z

This looks good, thank you! I’m not sure the Node behavior fully makes sense here (maybe we should throw inside of these methods) but at least I’m glad to get those tests in, and we can iterate on this further. cc @sebmarkbage if he has concerns.

facebook-github-bot added the CLA Signed label Apr 10, 2017

ManasJayanth force-pushed the patch-9102 branch 3 times, most recently from 5ace1a8 to c751f2a Compare April 16, 2017 10:07

ManasJayanth changed the title ~~[WIP] Lazy rAF check~~ Lazy rAF check Apr 16, 2017

ManasJayanth force-pushed the patch-9102 branch from c751f2a to da44a1e Compare April 17, 2017 03:17

ManasJayanth force-pushed the patch-9102 branch 2 times, most recently from 1a459cc to ce00e20 Compare April 18, 2017 19:44

ManasJayanth commented Apr 20, 2017

View reviewed changes

gaearon reviewed Apr 20, 2017

View reviewed changes

gaearon requested changes Apr 20, 2017

View reviewed changes

ManasJayanth force-pushed the patch-9102 branch 3 times, most recently from e99eb0a to fc0972d Compare April 20, 2017 17:09

gaearon reviewed Apr 21, 2017

View reviewed changes

ManasJayanth force-pushed the patch-9102 branch from 16d8d3a to d9eaa9a Compare April 21, 2017 14:08

gaearon reviewed Apr 21, 2017

View reviewed changes

ManasJayanth force-pushed the patch-9102 branch from d9eaa9a to fe4187d Compare April 21, 2017 15:05

ManasJayanth force-pushed the patch-9102 branch from bbcc27d to aaf2d1a Compare April 21, 2017 16:52

ManasJayanth force-pushed the patch-9102 branch 2 times, most recently from 93ed2b6 to abf5cc2 Compare April 25, 2017 15:14

ManasJayanth force-pushed the patch-9102 branch 2 times, most recently from 33f3ebc to a6ca10a Compare April 25, 2017 16:31

gaearon reviewed Apr 25, 2017

View reviewed changes

ManasJayanth force-pushed the patch-9102 branch from a6ca10a to 0729812 Compare April 26, 2017 13:47

Fixes facebook#9102 by fake polyfilling rAF (and rIC)

63ccd6f

ManasJayanth force-pushed the patch-9102 branch from 0729812 to 63ccd6f Compare April 26, 2017 13:50

gaearon added 2 commits April 27, 2017 11:57

Ensure we restore globals even if test fails + minor nits

be00694

Remove periods

27d895f

gaearon approved these changes Apr 27, 2017

View reviewed changes

gaearon changed the title ~~Lazy rAF check~~ Fiber ReactDOM shouldn't throw on import in Node environment if it's unused Apr 27, 2017

gaearon merged commit 182642b into facebook:master Apr 27, 2017

Fiber ReactDOM shouldn't throw on import in Node environment if it's unused #9389

Fiber ReactDOM shouldn't throw on import in Node environment if it's unused #9389

Conversation

ManasJayanth commented Apr 10, 2017 • edited Loading

ManasJayanth commented Apr 18, 2017 • edited Loading

gaearon commented Apr 18, 2017

ManasJayanth commented Apr 18, 2017

gaearon commented Apr 18, 2017

Choose a reason for hiding this comment

ManasJayanth Apr 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaearon left a comment

Choose a reason for hiding this comment

ManasJayanth commented Apr 21, 2017

gaearon commented Apr 21, 2017

ManasJayanth commented Apr 21, 2017

gaearon commented Apr 21, 2017

Choose a reason for hiding this comment

gaearon commented Apr 21, 2017

ManasJayanth commented Apr 21, 2017

ManasJayanth commented Apr 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ManasJayanth commented Apr 23, 2017 • edited Loading

ManasJayanth commented Apr 25, 2017

gaearon commented Apr 25, 2017 • edited Loading

ManasJayanth commented Apr 25, 2017 • edited Loading

gaearon commented Apr 25, 2017

ManasJayanth commented Apr 25, 2017

ManasJayanth commented Apr 25, 2017

gaearon commented Apr 25, 2017

ManasJayanth commented Apr 25, 2017 • edited Loading

ManasJayanth commented Apr 25, 2017

gaearon commented Apr 25, 2017

ManasJayanth commented Apr 25, 2017

ManasJayanth commented Apr 25, 2017 • edited Loading

Choose a reason for hiding this comment

ManasJayanth Apr 27, 2017 • edited Loading

Choose a reason for hiding this comment

gaearon commented Apr 27, 2017

ManasJayanth commented Apr 10, 2017 •

edited

Loading

ManasJayanth commented Apr 18, 2017 •

edited

Loading

ManasJayanth Apr 20, 2017 •

edited

Loading

ManasJayanth commented Apr 23, 2017 •

edited

Loading

gaearon commented Apr 25, 2017 •

edited

Loading

ManasJayanth commented Apr 25, 2017 •

edited

Loading

ManasJayanth commented Apr 25, 2017 •

edited

Loading

ManasJayanth commented Apr 25, 2017 •

edited

Loading

ManasJayanth Apr 27, 2017 •

edited

Loading