Replaced network.onRequestFinished() caching with network.getHAR() so that we can avoid redundantly (pre) caching JavaScript content. In the event that the HAR log doesn't contain a match, we'll fall back to fetching from the Network (and hoping for a cache hit from that layer).
I've tested both internally (internal Facebook DEV server) and externally (Code Sandbox) and it seems like this approach results in cache hits, so long as DevTools is opened when the page loads. (Otherwise it falls back to fetch().)
* update error message to include useLayoutEffect or useEffect on bad effect return
* Update packages/react-reconciler/src/ReactFiberCommitWork.new.js
Co-authored-by: Ricky <[email protected]>
* use existing import
Co-authored-by: Ricky <[email protected]>
Reverts part of #22275 (adding .catch() to Thenables in Suspense caches) in response to #22275 (comment).
After taking another look with fresh eyes, I think I see the "uncaught error" issue I initially noticed was in checkForUpdate() (which did not pass an error handler to .then)
This commit builds on PR #22260 and makes the following changes:
* Adds a DevTools feature flag for named hooks support. (This allows us to disable it entirely for a build via feature flag.)
* Adds a new Suspense cache for dynamically imported modules. (This allows a component to suspend while importing an external code chunk– like the hook names parsing code).
* DevTools supports a hookNamesModuleLoaderFunction param to import the hook names module. I wish this could be handles as part of the react-devtools-shared package, but I'm not sure how to configure Webpack (4) to serve the chunk from react-devtools-inline. This seemed like a reasonable workaround.
The PR also contains an additional unrelated change:
* Removes pre-fetch optimization (added in DevTools: Improve named hooks network caching #22198). This optimization was mostly only important for cases where sources needed to be re-downloaded, something which we can now avoid in most cases¹ thanks to using cached responses already loaded by the page. (I tested this locally on Facebook and this change has no negative performance impact. There is still some overhead from serializing the JS through the Bridge but that's constant between the two approaches.)
¹ The case where we don't benefit from cached responses is when DevTools are opened after the page has already loaded certain scripts. This seems uncommon enough that I don't think it justified the added complexity of prefetching.
* Add test that triggers infinite update loop
In 18, passive effects are flushed synchronously if they are the
result of a synchronous update. We have a guard for infinite update
loops that occur in the layout phase, but it doesn't currently work for
synchronous updates from a passive effect.
The reason this probably hasn't come up yet is because synchronous
updates inside the passive effect phase are relatively rare: you either
have to imperatively dispatch a discrete event, like `el.focus`, or you
have to call `ReactDOM.flushSync`, which triggers a warning. (In
general, updates inside a passive effect are not encouraged.)
I discovered this because `useSyncExternalStore` does sometimes
trigger updates inside the passive effect phase.
This commit adds a failing test to prove the issue exists. I will fix
it in the next commit.
* Fix failing test added in previous commit
The way we detect a "nested update" is if there's synchronous work
remaining at the end of the commit phase.
Currently this check happens before we synchronously flush the passive
effects. I moved it to after the effects are fired, so that it detects
whether synchronous work was scheduled in that phase.
* Test bad useEffect return value with noop-renderer
* Use previous "root"-approach
Tests should now be invariant under variants
* Add same test for layout effects
* Fix#21972: Add `onResize` event to video elements
This is a simple fix for #21972 that adds support for the `onResize` media event.
I created a separate `videoEventTypes` array since I doubt anyone will want to add `onResize` to an audio event. It would simplify the code a bit to just add `resize` to the `mediaEventTypes` array, if that’s preferred.
Pre-commit checklist ([source](https://reactjs.org/docs/how-to-contribute.html#sending-a-pull-request))
✅ Fork the repository and create your branch from `main`.
✅ Run `yarn` in the repository root.
✅ If you’ve fixed a bug or added code that should be tested, add tests!
✅ Ensure the test suite passes (`yarn test`). Tip: `yarn test --watch TestName` is helpful in development.
✅ Run `yarn test --prod` to test in the production environment.
✅ If you need a debugger, run `yarn debug-test --watch TestName`, open chrome://inspect, and press “Inspect”.
✅ Format your code with prettier (`yarn prettier`).
✅ Make sure your code lints (`yarn lint`). Tip: `yarn linc` to only check changed files.
✅ Run the Flow typechecks (`yarn flow`).
✅ If you haven’t already, complete the CLA.
* Consolidate `videoEventTypes` array into `mediaEventTypes`
Adds support for useSyncExternalStore to react-debug-tools, which in
turn adds support for React Devtools.
Test plan: I added a test to ReactHooksInspectionIntegration, based on
existing one for useMutableSource.
The userspace shim of useSyncExternalStore uses a useState hook because
it's the only way to trigger a re-render. We don't actually use the
queue to store anything, because we read the current value directly
from the store.
In the native implementation, we can schedule an update on the fiber
directly, without the overhead of a queue.
This adds an initial implementation of useSyncExternalStore to the
fiber reconciler. It's mostly a copy-paste of the userspace
implementation, which is not ideal but is a good enough starting place.
The main change we'll want to make to this native implementation is to
move the tearing checks from the layout phase to an earlier, pre-commit
phase so that code that runs in the commit phase always observes a
consistent tree.
Follow-ups:
- Implement in Fizz
- Implement in old SSR renderer
- Implement in react-debug-hooks
This sets up an initial shim implementation of useSyncExternalStore,
via the use-sync-external-store package. It's designed to mimic the
behavior of the built-in API, but is backwards compatible to any version
of React that supports hooks.
I have not yet implemented the built-in API, but once it exists, the
use-sync-external-store package will always prefer that one. Library
authors can depend on the shim and trust that their users get the
correct implementation.
See https://github.com/reactwg/react-18/discussions/86 for background
on the API.
The tests I've added here are designed to run against both the shim and
built-in implementation, using our variant test flag feature. Tests that
only apply to concurrent roots will live in a separate suite.
While testing the recently-launched named hooks feature, I noticed that one of the two big performance bottlenecks is fetching the source file. This was unexpected since the source file has already been loaded by the page. (After all, DevTools is inspecting a component defined in that same file.)
To address this, I made the following changes:
- [x] Keep CPU bound work (parsing source map and AST) in a worker so it doesn't block the main thread but move I/O bound code (fetching files) to the main thread.
- [x] Inject a function into the page (as part of the content script) to fetch cached files for the extension. Communicate with this function using `eval()` (to send it messages) and `chrome.runtime.sendMessage()` to return its responses to the extension).
With the above changes in place, the extension gets cached responses from a lot of sites- but not Facebook. This seems to be due to the following:
* Facebook's response headers include [`vary: 'Origin'`](https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Vary).
* The `fetch` made from the content script does not include an `Origin` request header.
To reduce the impact of cases where we can't re-use the Network cache, this PR also makes additional changes:
- [x] Use `devtools.network.onRequestFinished` to (pre)cache resources as the page loads them. This allows us to avoid requesting a resource that's already been loaded in most cases.
- [x] In case DevTools was opened _after_ some requests were made, we also now pre-fetch (and cache in memory) source files when a component is selected (if it has hooks). If the component's hooks are later evaluated, the source map will be faster to access. (Note that in many cases, this prefetch is very fast since it is returned from the disk cache.)
With the above changes, we've reduced the time spent in `loadSourceFiles` to nearly nothing.
lunaruan commented 3 days ago •
This PR adds separate DevTools feature flag configurations for react-devtools-core. It also breaks the builds down into facebook specific and open source flags so we can experiment in React Native.
Tested yarn build:standalone, yarn build:backend, yarn build:standalone:fb, and yarn build:backend:fb and inspected the output to make sure each package used the correct feature flags (the first two use core-oss and the latter two use fb-oss.
React currently suppress console logs in StrictMode during double rendering. However, this causes a lot of confusion. This PR moves the console suppression logic from React into React Devtools. Now by default, we no longer suppress console logs. Instead, we gray out the logs in console during double render. We also add a setting in React Devtools to allow developers to hide console logs during double render if they choose.
