Remove the 'alwaysUseRequestIdleCallbackPolyfill' feature flag #12648
Conversation
**what is the change?:** Removes the feature flag 'alwaysUseRequestIdleCallbackPolyfill', such that we **always** use the polyfill for requestIdleCallback. **why make this change?:** We have been testing this feature flag at 100% for some time internally, and determined it works better for React than the native implementation. Looks like RN was overriding the flag to use the native when possible, but since no RN products are using 'async' mode it should be safe to switch this flag over for RN as well. **test plan:** We have already been testing this internally for some time. **issue:** internal task t28128480
|
ReactDOM: size: -0.1%, gzip: 0.0% Details of bundled changes.Comparing: 999b656...c65cc2f react-art
react-dom
react-scheduler
Generated by 🚫 dangerJS |
|
Surprised to find that this does seem to cause tests to fail. So the first step in this project is to fix the current polyfill so that our tests pass. |
|
RN uses ReactNativeFrameScheduling so I don't think this should matter for Fabric (which is or going to be async). |
|
@flarnie It might not be a bug in the polyfill. Our tests have been running using these mocks: global.requestAnimationFrame = function(callback) {
setTimeout(callback);
};
global.requestIdleCallback = function(callback) {
return setTimeout(() => {
callback({
timeRemaining() {
return Infinity;
},
});
});
};which of course is not how the polyfill works :D |
|
There are also test modules like this one that mock react/packages/react-dom/src/__tests__/ReactDOMRoot-test.js Lines 28 to 67 in 999b656 These will also need to be updated. I don't like mocking, generally, but seems like we need the ability to mock the Scheduler module in some cases. |
|
The failing test passes locally.
Looking into it more now. |
|
If we mock the Scheduler, we can't run these as production tests ever which seems unfortunate. We should opt to mock the DOM environment if possible. |
|
I could write mocks for rAF and postMessage, but we still need some way to trigger a 'flush' of those. Similar to our other tests for async, we need a way to control simulated time passing. "we can't run these as production tests ever" - can you explain that more? |
|
The bundle tests are failing. You can reproduce by doing We run the subset of tests on bundles themselves. This is beneficial to catch regression in our build process. As a last resort you can rename a test to |
|
Ah - I think I understand now what you meant @sebmarkbage by "we can't run these as production tests ever". Let me verify; The test which uses the bundle fails because you can't mock out dependencies which are already rolled up into a bundle. Calling Let me know if I'm missing something. I think I can make this work with some clever mocking of rAF/postMessage and writing a 'flush' helper that coordinates with the mocks. |
**what is the change?:** - In all tests where we previously mocked rIC or relied on native mocking which no longer works, we are now mocking rAF and postMessage. - Also adds a basic initial test for ReactScheduler. NOTE -> we do plan to write headless browser tests for ReactScheduler! This is just an initial test, to verify that it works with the mocked out browser APIs as expected. **why make this change?:** We need to mock out the browser APIs more completely for the new 'ReactScheduler' to work in our tests. Many tests are depending on it, since it's used at a low level. By mocking the browser APIs rather than the 'react-scheduler' module, we enable testing the production bundles. This approach is trading isolation for accuracy. These tests will be closer to a real use. **test plan:** run the tests :) **issue:** internal task T28128480
flarnie
force-pushed
the
removeFlagForRICPolyfilling
branch
from
b0f1903
to
c65cc2f
Compare
|
Added a new approach - just mocking browser APIs. This is a significant change, would appreciate a review of the latest commit. Thanks @sebmarkbage @acdlite @gaearon for the input so far. |
flarnie
dismissed stale reviews from gaearon and bvaughn
added new logic, latest commit needs review
|
We're mocking the browser APIs instead of the module, so we can keep running tests on the production bundles. ☀️ |
|
I don't understand the argument for mocking the browser APIs. That only seems useful if the mocks replicate the browser exactly. That's not how these mocks are written — if you ran these tests in the browser, without the mocks, the tests would fail. So then what's the point? The reason I thought we should mock the scheduler APIs is so you can write async tests that lazily flush work without worrying about the actual details of how the work is flushed in a real environment. The same reason we have ReactNoop. |
|
@acdlite made a good point. If we treat the Scheduler as an API external to React, then that is the public API boundary. It is mockable even with bundled code. However, we should make sure it works with UMD bundles where the Scheduler is external too. |
Sounds good, I'd like to tackle that in a follow-up PR. Sounds like these are the action items: Going to find one of you in person tomorrow to talk through this. |
Edit:
When we ran the tests, it became clear that tests were relying on the old version using rIC and the way that either our tests or Jest was polyfilling rIC.
In the new version of this scheduler, we rely on browser APIs which either aren't fully mocked or aren't mocked at all in the tests by default, so the tests failed.
This PR now updates all relevant tests to mock out the browser APIs used by ReactScheduler, and also adds an initial test for ReactScheduler to verify it works in the Jest test environment with the mocked browser APIs.
A follow-up PR will pull the mocking logic into a reusable helper
what is the change?:
Removes the feature flag 'alwaysUseRequestIdleCallbackPolyfill', such
that we always use the polyfill for requestIdleCallback.
why make this change?:
We have been testing this feature flag at 100% for some time internally,
and determined it works better for React than the native implementation.
Looks like RN was overriding the flag to use the native when possible,
but since no RN products are using 'async' mode it should be safe to
switch this flag over for RN as well.
test plan:
We have already been testing this internally for some time.
issue:
internal task t28128480