Clean up Scheduler forks #20915
Merged
Clean up Scheduler forks #20915
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
facebook-github-bot
added
the
React Core Team
rickhanlonii
commented
Mar 2, 2021
rickhanlonii
force-pushed
the
rh-refac-scheduler
branch
from
612e740 to
4e91985
Compare
|
Tests are failing because GCC does not like using the setTimeout global
|
|
It's because you have a local variable that's called the same thing. Rename to something else ( |
|
Comparing: 1a2d792...0bbcfc2 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show
|
gaearon
reviewed
Mar 2, 2021
| const localClearTimeout = | ||
| typeof window !== 'undefined' ? window.clearTimeout : clearTimeout; | ||
| const localSetImmediate = | ||
| typeof window !== 'undefined' ? window.setImmediate : setImmediate; // IE and Node.js + jsdom |
There was a problem hiding this comment.
This will insta-crash non-browser environments that don't have setImmediate global.
I suggest this pattern:
let localSetTimeout = typeof setTimeout === 'function' ? setTimeout : undefined;
let localClearTimeout = typeof clearTimeout === 'function' ? clearTimeout : undefined;
let localSetImmediate = typeof setImmediate === 'function' ? setImmediate : undefined;
There was a problem hiding this comment.
I'd recommend not hoisting these up like this and instead move them into where they're used so that the variable always has a consistent type. No need for the "undefined" intermediate step.
For example:
if (typeof setImmediate === 'function') {
const localSetImmediate = setImmediate
schedulePerformWorkUntilDeadline = () => {
localSetImmediate(performWorkUntilDeadline);
};
} else {or even
if (typeof setImmediate === 'function') {
schedulePerformWorkUntilDeadline = () => {
setImmediate(performWorkUntilDeadline);
};
} else {There was a problem hiding this comment.
setTimeout and clearTimeout are not only used at the top level, so how would you handle that without adding a runtime check cost?
gaearon
requested changes
Mar 2, 2021
|
Do we know why CI fails? |
|
@gaearon yes, it's failing because the test is now using it('resets correctly across renderers', async () => {
function Effecty() {
React.useEffect(() => {}, []);
return null;
}
TestUtils.act(() => {
TestRenderer.act(() => {});
expect(() => {
TestRenderer.create(<Effecty />);
}).toWarnDev(["It looks like you're using the wrong act()"], {
withoutStack: true,
});
});
});to throw this error:
Note that changing the test to use async act fixes the failure, but I'm concerned about just doing that without understanding the cause. |
|
I have a vague memory of (maybe?) writing that error message a few years ago but I have no recollection of what it was about. I’ll need to take a closer look. |
rickhanlonii
force-pushed
the
rh-refac-scheduler
branch
from
4d72bab to
19f6bc7
Compare
|
So I confirmed that this failure is due to differences between setTimeout and setImmediate in Node and Jest. For example, consider this test (repl): it("crashes", () => {
let document = {};
setTimeout(() => {
// This is never hit
document.foo;
});
setImmediate(() => {
// This is hit and will crash the test
document.foo
});
document = null;
});Here, the setTimeout will not crash the test, but setImmediate will. I believe this is happening because of how the Node event loop processes setImmediate vs setTimeout. Specifically, setTimeout is processed after the close callbacks and after Jest cleans up the test environment. But setImmediate is called before close callbacks, and after Jest cleans up the test. The result is that work scheduled in setTimeout is not processed, but work in setImmediate it. Since Jest has already cleaned up the environment, the document is gone, and we error. The fix here is to use async act so that Jest doesn't clean up the environment before all of the work is flushed. That turns the above into essentially this, which passes: it("should pass", async () => {
let document = {};
setTimeout(() => {
// This is never hit
document.foo;
});
await new Promise((resolve) => {
setImmediate(() => {
// This is hit but the document is available
document.foo;
resolve();
});
});
document = null;
});That fixes the issue here, but makes the setImmediate change more of a breaking change than expected. |
|
I've reduced this down to this test, which shows it's happens when:
jest.useRealTimers();
it('fails', () => {
function Effecty() {
React.useEffect(() => {}, []);
return null;
}
TestRenderer.create(<Effecty />);
// Or:
// ReactDOM.render(<Effecty />, document.createElement('div'));
});This will fail both internally and in the fixture (in different ways where document is not defined). |
rickhanlonii
force-pushed
the
rh-refac-scheduler
branch
from
039b0bf to
7dfdd14
Compare
acdlite
approved these changes
May 17, 2021
facebook-github-bot
pushed a commit
to facebook/react-native
that referenced
this pull request
May 25, 2021
Summary: This sync includes the following changes: - **[316943091](facebook/react@316943091 )**: Make StrictMode double rendering flag static for FB/www ([#21517](facebook/react#21517)) //<Brian Vaughn>// - **[e0f89aa05](facebook/react@e0f89aa05 )**: Clean up Scheduler forks ([#20915](facebook/react#20915)) //<Ricky>// - **[5890e0e69](facebook/react@5890e0e69 )**: Remove data-reactroot from server rendering and hydration heuristic ([#20996](facebook/react#20996)) //<Sebastian Markbåge>// - **[46491dce9](facebook/react@46491dce9 )**: [Bugfix] Prevent already-committed setState callback from firing again during a rebase ([#21498](facebook/react#21498)) //<Andrew Clark>// - **[b770f7500](facebook/react@b770f7500 )**: lint-build: Infer format from artifact filename ([#21489](facebook/react#21489)) //<Andrew Clark>// - **[2bf4805e4](facebook/react@2bf4805e4 )**: Update entry point exports ([#21488](facebook/react#21488)) //<Brian Vaughn>// Changelog: [General][Changed] - React Native sync for revisions b8fda6c...ebcec3c jest_e2e[run_all_tests] Reviewed By: JoshuaGross Differential Revision: D28572047 fbshipit-source-id: eb09c0358edb7fbf241333ea9c08724748906fea
koto
pushed a commit
to koto/react
that referenced
this pull request
Jun 15, 2021
* Clean up Scheduler forks * Un-shadow variables * Use timer globals directly, add a test for overrides * Remove more window references * Don't crash for undefined globals + tests * Update lint config globals * Fix test by using async act * Add test fixture * Delete test fixture
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
This PR cleans up the scheduler files to reduce bundle size, support more environments correctly, and allow for better layering in the future.
Details
Previously, we had a scheduler for DOM environments (
SchedulerDOM) and a scheduler for non-DOM environments (SchedulerNoDom). In this model, users would select theSchedulerthey needed based on the environment they used.There were a few problems with this approach. SchedulerDOM included the MessageChannel implementation which should be used outside of DOM environments (e.g. Worker environments). Second, in practice we were still shipping feature detection to select between different environments, which added bundle size without achieving the desired layering. Finally, we've since refactored these two Schedulers such that the only difference between them is how they schedule work in the event loop. This means the files are identical except for a few lines, so the added bundle size is unnecessary.
The updated structure includes the following forks:
Scheduler: The current scheduler implementation which will usesetImmediate,MessageChannel, orsetTimeout, in that order, based on what is globally available.SchedulerPostTask: An experimental, internal-only scheduler which is a different implementation based on the new ChromepostTaskAPI. This does not add bundle size because it is not exposed.SchedulerMock: An internal-only scheduler for testing. Also not exposed.