Retries should be allowed to expire if they are CPU bound for too long,
but when I made this change it caused a spike in browser crashes. There
must be some other underlying bug; not super urgent but ideally should
figure out why and fix it. Unfortunately we don't have a repro for the
crashes, only detected via production metrics.
React has its own component stack generation code that DevTools embeds a fork of, but both of them use a shared helper for disabling console logs. This shared helper is DEV only though, because it was intended for use with React DEV-only warnings and we didn't want to unnecessarily add bytes to production builds.
But DevTools itself always ships as a production build– even when it's used to debug DEV bundles of product apps (with third party DEV-only warnings). That means this helper was always a noop.
The resolveCurrentDispatcher method was changed recently to replace the thrown error with a call to console.error. This newly logged error ended up slipping through and being user visible because of the above issue.
This PR updates DevTools to also fork the console patching logic (to remove the DEV-only guard).
Note that I didn't spot this earlier because my test harness (react-devtools-shell) always runs in DEV mode. 🤡
This may not be the first root element if the root is a fragment and the
second one unsuspends first. But this tag doesn't work well for root
fragments anyway.
* Fix reentrancy bug
* Fix another reentrancy bug
There's also an issue if we try to schedule something to be client
rendered if its fallback hasn't rendered yet. So we don't do it
in that case.
* Add NewContext module
This implements a reverse linked list tree containing the previous
contexts.
* Implement recursive algorithm
This algorithm pops the contexts back to a shared ancestor on the way down
the stack and then pushes new contexts in reverse order up the stack.
* Move isPrimaryRenderer to ServerFormatConfig
This is primarily intended to be used to support renderToString with a
separate build than the main one. This allows them to be nested.
* Wire up more element type matchers
* Wire up Context Provider type
* Wire up Context Consumer
* Test
* Implement reader in class
* Update error codez
My personal workflow is to develop against the www-modern release
channel, with the variant flags enabled, because it encompasses the
largest set of features. Then I rely on CI to run the tests against
all the other configurations.
So in practice, I almost always run
```
yarn test -r=www-modern --variant TEST_FILE
```
instead of
```
yarn test TEST_FILE
```
So, I've updated the `yarn test` command to use those options
by default.
I recently started using `pendingPassiveEffectsLanes` to check if there were any pending
passive effects (530027a). `pendingPassiveEffectsLanes` is the value of
`root.finishedLanes` at the beginning of the commit phase. When there
are pending passive effects, it should always be a non-zero value,
because it represents the lanes used to render the effects.
But it turns out that `root.finishedLanes` isn't always correct.
Sometimes it's `NoLanes` even when there's a new commit.
I found this while investigating an internal bug report. The only repro
I could get was via a headless e2e test runner; I couldn't get one in an
actual browser, or other interactive environment. I used the e2e test to
bisect and confirm the fix. But I don't know yet know how to write a
regression test for the precise underlying scenario. I can probably
reverse engineer one by studying the code; after a quick glance
at `performConcurrentWorkOnRoot` and `performSyncWorkOnRoot`, it's not
hard to see how this might happen.
In the meantime, I'll revert the recent change that exposed the bug.
I was surprised that this had never come up before, since the code that
assigns `root.finishedLanes` is in an extremely hot path, and it hasn't
changed in a while. The reason is that, before 530027a,
`root.finishedLanes` was only used by the DevTools profiler, which is
probably why we had never noticed any issues. In addition to fixing the
inconsistency, we might also consider making `finishedLanes` a
profiling-only field.
We assume that isArray and getIteratorFn are only called on objects.
So we shouldn't have to check that again and again, and then check a flag.
We can just stay in this branch.
There is a slight semantic breakage here because you could have an
iterator on a function, such as if it's a generator function. But that's
not supported and that currently only works at the root. The inner slots
don't support this.
So this just makes it consistent.
Tracked Fibers are called "updaters" and are exposed to DevTools via a 'memoizedUpdaters' property on the ReactFiberRoot. The implementation of this feature follows a vaguely similar approach as interaction tracing, but does not require reference counting since there is no subscriptions API.
This change is in support of a new DevTools Profiler feature that shows which Fiber(s) scheduled the selected commit in the Profiler.
All changes have been gated behind a new feature flag, 'enableUpdaterTracking', which is enabled for Profiling builds by default. We also only track updaters when DevTools has been detected, to avoid doing unnecessary work.
I recently added UI for the Profiler's commit and post-commit durations to the DevTools, but I made two pretty silly oversights:
1. I used the commit hook (called after mutation+layout effects) to read both the layout and passive effect durations. This is silly because passive effects may not have flushed yet git at this point.
2. I didn't reset the values on the HostRoot node, so they accumulated with each commit.
This commitR addresses both issues:
1. First it adds a new DevTools hook, onPostCommitRoot*, to be called after passive effects get flushed. This gives DevTools the opportunity to read passive effect durations (if the build of React being profiled supports it).
2. Second the work loop resets these durations (on the HostRoot) after calling the post-commit hook so address the accumulation problem.
I've also added a unit test to guard against this regressing in the future.
* Doing this in flushPassiveEffectsImpl seemed simplest, since there are so many places we flush passive effects. Is there any potential problem with this though?
I screwed this up in #21082. Got confused by the < versus > thing again.
The helper functions are annoying, too, because I always forget the
intended order of the arguments. But they're still helpful because when
we refactor the type we only have the change the logic in one place.
Added a regression test.
Ricky
Brian Vaughn
Andrew Clark
Sebastian Markbåge
Henry Q. Dineen
Paul O’Shannessy
Joshua Gross
inokawa