In investigating the results of #30572 we discovered a lot of false positives from when functions containing possibly-effectful calls were returned from hooks or passed into hooks. This corresponds to similar issues for GlobalMutations, because the effectful call work uses the same FunctionEffect infra. This fixes the infra to allow GlobalMutation function effects in return and throw terminals and in all hook arguments.
[ghstack-poisoned]
[`react-window` disables `pointerEvents` while scrolling meaning you
can't click anything while
scrolling.](https://github.com/bvaughn/react-window/issues/128).
This means that the first click when you stop the scroll with inertial
scrolling doesn't get registered. This is suuuper annoying. This might
make sense when you click to stop on a more intentional UI but it
doesn't makes sense in a list like this because we eagerly click things
even on mousedown.
This PR just override that to re-enable pointer events.
Supposedly this is done for performance but that might be outdated
knowledge. I haven't observed any difference so far.
If we discover that it's a perf problem, there's another technique we
can use where we call `ownerDocument.elementFromPoint(e.pageX, e.pageY)`
and then dispatch the event against that element. But let's try the
simplest approach first?
There's two problems. The biggest one is that it turns out that Chrome
is throttling looping timers that we're using both while polling and for
batching bridge traffic. This means that bridge traffic a lot of the
time just slows down to 1 second at a time. No wonder it feels sluggish.
The only solution is to not use timers for this.
Even when it doesn't like in Firefox the batching into 100ms still feels
too sluggish.
The fix I use is to batch using a microtask instead so we can still
batch multiple commands sent in a single event but we never artificially
slow down an interaction.
I don't think we've reevaluated this for a long time since this was in
the initial commit of DevTools to this repo. If it causes other issues
we can follow up on those.
We really shouldn't use timers for debouncing and such. In fact, React
itself recommends against it because we have a better technique with
scheduling in Concurrent Mode. The correct way to implement this in the
bridge is using a form of back-pressure where we don't keep sending
messages until we get a message back and only send the last one that
matters. E.g. when moving the cursor over a the elements tab we
shouldn't let the backend one-by-one move the DOM node to each one we
have ever passed. We should just move to the last one we're currently
hovering over. But this can't be done at the bridge layer since it
doesn't know if it's a last-one-wins or imperative operation where each
one needs to be sent. It needs to be done higher. I'm not currently
seeing any perf problems with this new approach but I'm curious on React
Native or some thing. RN might need the back-pressure approach. That can
be a follow up if we ever find a test case.
Finally, the other problem is that we use a Suspense boundary around the
Element Inspection. Suspense boundaries are for things that are expected
to take a long time to load. This shows a loading state immediately. To
avoid flashing when it ends up being fast, React throttles the reveal to
200ms. This means that we take a minimum of 200ms to show the props. The
way to show fast async data in React is using a Transition (either using
startTransition or useDeferredValue). This lets the old value remaining
in place while we're loading the next one.
We already implement this using `inspectedElementID` which is the async
one. It would be more idiomatic to implement this with useDeferredValue
rather than the reducer we have now but same principle. We were just
using the wrong ID in a few places so when it synchronously updated they
suspended. So I just made them use the inspectedElementID instead.
Then I can simply remove the Suspense boundary. Now the selection
updates in the tree view synchronously and the sidebar lags a frame or
two but it feels instant. It doesn't flash to white between which is
key.
Summary:
This diff extends the existing work on validating against locals being reassigned after render, by propagating the reassignment "effect" into the lvalues of instructions when the rvalue operands include values known to cause reassignments. In particular, this "closes the loop" for function definitions and function calls: a function that returns a function that reassigns will be considered to also perform reassignments, but previous to this we didn't consider the result of a `Call` of a function that reassigns to itself be a value that reassigns.
This causes a number of new bailouts in test cases, all of which appear to me to be legit.
ghstack-source-id: 770bf02d07
Pull Request resolved: https://github.com/facebook/react/pull/30540
When aborting with a postpone value in Fizz if any tasks are still
pending in the root while prerendering the prerender will fatally error.
This is different from postponing imperatively in a root task and really
the semantics should be the same. This change updates React to treat an
abort with a postpone value as a postponed root rather than a fatal
error.
This just tracks the `.parent` field properly and uses DevToolsInstances
in more places that used to use IDs or Fibers.
I also use this new parent path when looking up a DevToolsInstance from
a DOM node. This should ideally be simple because the `.parent` field
represents only the unfiltered parents and include any virtual parents.
So we should be able to just get one from nearest Fiber that has one.
However, because we don't currently always clean up the map of
DevToolsInstances (e.g. updateComponentFilters doesn't recursively clean
out everything) it can leave matches hanging that shouldn't be there. So
we need to run the shouldFilterFiber filter to ignore those.
Another interesting implication is that without a FiberInstance we don't
have a way to get to a VirtualInstance from a HostComponent. Which means
that even filtered Fibers need to have a FiberInstance if they have a
VirtualInstance parent. Even if we don't actually mount them into the
front-end.
There's a special case that happens when we replay logs on the client
because this doesn't happen within the context of any particular
rendered component. So we need to reimplement things that would normally
be handled by a full client like Fiber.
The implementation of `getOwnerStackByComponentInfoInDev` is the
simplest version since it doesn't have any client components in it so I
move it to `shared/`. It's only used by Flight but both `react-server/`
and `react-client/` packages. The ReactComponentInfo type is also more
generic than just Flight anyway.
In a follow up I still need to implement this in React DevTools when
native tasks are not available so that it appends it to the console.
While debugging #30536 I happened to notice that the bug only reproduced
when there was interleaving scopes, and observed that an unpruned scope
nested inside of a pruned one was not being visited by
CollectPromotableTemporaries, which keeps track of which identifiers
should be promoted later. Therefore when actually promoting temporaries
we were skipping over the identifiers in children of pruned scopes
ghstack-source-id: d805f62f22
Pull Request resolved: https://github.com/facebook/react/pull/30537
Stacked on #30494 and #30491.
This is setting us up to be able to track Server Components. This is now
split into a FiberInstance (Client Component) and a VirtualInstance
(Server Component). We're not actually creating any VirtualInstances yet
though this is just the data structures.
Server Components and potentially other compiled away or runtime
optimized away (e.g. calling through a function without creating an
intermediate Fiber) don't have a stateful instance. They just represent
the latest data. It's kind of like a React Element.
However, in the DevTools UI we need them to be stateful partly just so
that you can select and refer to them separately. E.g. the same Server
Component output rendered into different slots on the client should
still have two different representations in the DevTools. Also if the
same child Fibers update in place because the Server Component refreshed
we shouldn't lose the selection if you've selected a Server Component.
I'll implement this by creating Virtual Instances that only exist for
the purpose of the DevTools UI and so it'll be implemented in the
DevTools.
We could just make a Map from `id` to `Fiber | ReactComponentInfo` but
that requires a branching without a consistent hidden class. We also
need some more states on there. We also have some other Maps that tracks
extra states like that of component stacks, errors and warnings.
Constantly resizing and adding/removing from a Map isn't exactly fast.
It's faster to have a single Map with an object in it than one Map per
object. However, having extra fields that are usually just `null` can
instead mean more memory gets used. Since only a very small fraction of
instances will have errors/warnings or having initialized its component
stack, it made sense to store that in a separate Map that is usually
just empty.
However, with the addition of particularly the `parent` field and the
ability to do a fast hidden-class safe branching on the `kind` field I
think it's probably worth actually allocating an extra first class
object per Instance to store DevTools state into. That's why I converted
from just storing `Fiber` -> `id` to storing `Fiber` ->
`DevToolsInstance` which then keeps the warnings/errors/componentStack
as extra fields that are usually `null`. That is a lot of objects though
since it's one per Fiber pair basically.
We use static dependency injection. We shouldn't use this dynamic
dependency injection we do for DevTools internals. There's also meta
programming like spreading and stuff that isn't needed.
This moves the config from `injectIntoDevTools` to the FiberConfig so it
can be statically resolved.
Closure Compiler has some trouble generating optimal code for this
anyway so ideally we'd refactor this further but at least this is better
and saves a few bytes and avoids some code paths (when minified).
Stacked on #30491.
When going from DOM Node to select a component or highlight a component
we find the nearest mounted ancestor. However, when multiple renderers
are nested there can be multiple ancestors. The original fix#24665 did
this by taking the inner renderer if it was an exact match but if it
wasn't it just took the first renderer.
Instead, we can track the inner most node we've found so far. Then get
the ID from that node (which will be fast since it's now a perfect
match). This is a better match.
However, the main reason I'm doing this is because the old mechanism
leaked the `Fiber` type outside the `RendererInterface` which is
supposed to abstract all of that. With the new algorithm this doesn't
leak.
I've tested this with a new build against the repro in the old issue
#24539 and it seems to work.
The invalid GraphQL in these fixtures somehow causes an unhandled
promise rejection error when running `yarn prettier-all`. This fixes
that issue by making the GraphQL valid.
With GitHub issue templates this workflow is not truly necessary and
can deny other workflows from running due to a limited amount of CI
workers in the pool.
I propose deleting this workflow and relying on issue templates instead.
ghstack-source-id: a798621f36
Pull Request resolved: https://github.com/facebook/react/pull/30518
Stacked on #30490.
This is in the same spirit but to clarify the difference between what is
React Native vs part of any generic Host. We used to use "Native" to
mean three different concepts. Now "Native" just means React Native.
E.g. from the frontend's perspective the Host can be
Highlighted/Inspected. However, that in turn can then be implemented as
either direct DOM manipulation or commands to React Native. So frontend
-> backend is "Host" but backend -> React Native is "Native" while
backend -> DOM is "Web".
Rename NativeElementsPanel to BuiltinElementsPanel. This isn't a React
Native panel but one part of the surrounding DevTools. We refer to Host
more as the thing running React itself. I.e. where the backend lives.
The runtime you're inspecting. The DevTools itself needs a third term.
So I went with "Builtin".
This is not used by DevTools since it has its own implementation of it.
This function is getting removed since `findDOMNode` is getting removed
so we shouldn't keep around extra bytes unnecessarily.
There is also `findHostInstancesForRefresh` which should really be
implemented on the `react-refresh` side. Not using an injection but
that's a heavier lift and only affects `__DEV__`.
Several CI workflows depend on the runtime_build_and_test.yml workflow
to complete before it can successfully download the build artifact.
However it is possible to encounter a race condition where the build
hasn't completed when the new workflow is started.
This PR adds a simple polling mechanism that waits up to 10 minutes for
the build for that revision to complete.
ghstack-source-id: 6a954638a8
Pull Request resolved: https://github.com/facebook/react/pull/30515
Added instructions on the new workflow. Let's keep this short around for
now as it may be used again due to muscle memory and emit a helpful
message instead.
ghstack-source-id: 3f840a3d43
Pull Request resolved: https://github.com/facebook/react/pull/30510
There is currently a mismatch in how the persistent mode JS API and the
Fabric native code interpret `completeRoot`.
This is a short-lived experiment to see the effect of moving the Fabric
`completeRoot` call from `finalizeContainerChildren` to
`replaceContainerChildren` which in some cases does not get called.
actions/cache has a default timeout of 10 minutes. Occasionally the
cache service download gets stuck and it waits this amount of time
before proceeding like it was a cache miss.
10 minutes is way too long so let's shorten this to a minute.
ghstack-source-id: 95dee31bd9
Pull Request resolved: https://github.com/facebook/react/pull/30512
Currently if you abort a Fizz render during rendering the render will
not complete correctly because there are inconsistencies with task
counting. This change updates the abort implementation to allow you to
abort from within a render itself. We already landed a similar change
for Flight in #29764
We need a GitHub token to download artifacts from GitHub so
unfortunately codesandboxci will need to revert to the slower process
of building from source for now until it's possible to pass secrets to
codesandboxci.
ghstack-source-id: edab979084
Pull Request resolved: https://github.com/facebook/react/pull/30511
I need to start clarifying where things are really actually Fibers and
where they're not since I'm adding Server Components as a separate type
of component instance which is not backed by a Fiber.
Nothing in the front end should really know anything about what kind of
renderer implementation we're inspecting and indeed it's already not
always a "Fiber" in the legacy renderer.
We typically refer to this as a "Component Instance" but the front end
currently refers to it as an Element as it historically grew from the
browser DevTools Elements tab.
I also moved the renderer.js implementation into the `backend/fiber`
folder. These are at the same level as `backend/legacy`. This clarifies
that anything outside of this folder ideally shouldn't refer to a
"Fiber".
console.js and profilingHooks.js unfortunately use Fibers a lot which
needs further refactoring. The profiler frontend also uses the term
alot.
When a Fizz render is closing but not yet closed it's possible that
pinged tasks can spawn more work. The point of the closing state is to
allow time to start piping/reading the underlying stream but
semantically the render is finished at that point so work should no
longer happen.
Mike Vitousek
Sebastian MarkbågeandGitHub
Sebastian SilbermannandGitHub
Josh StoryandGitHub
Hendrik LiebauandGitHub
Jack PopeandGitHub
danandGitHub
Lauren Tan
Mofei Zhang
Jan KassensandGitHub