Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[pull] main from facebook:main #155

Merged
merged 2,761 commits into from
Oct 9, 2024
Merged

[pull] main from facebook:main #155

merged 2,761 commits into from
Oct 9, 2024

Conversation

pull[bot]
Copy link

@pull pull bot commented May 16, 2024

See Commits and Changes for more details.


Created by pull[bot]

Can you help keep this open source service alive? 💖 Please sponsor : )

@pull pull bot added the ⤵️ pull label May 16, 2024
mvitousek and others added 29 commits August 16, 2024 13:27
Summary:
Refs, as stable values that the rules of react around mutability do not apply to, currently are treated as having mutable ranges, and through aliasing, this can extend the mutable range for other values and disrupt good memoization for those values. This PR excludes refs and their .current values from having mutable ranges.

Note that this is unsafe if ref access is allowed in render: if a mutable value is assigned to ref.current and then ref.current is mutated later, we won't realize that the original mutable value's range extends.

ghstack-source-id: e8f36ac25e2c9aadb0bf13bd8142e4593ee9f984
Pull Request resolved: #30713
…llback

Test Plan:
Documents that useCallback calls interfere with it being ok for refs to escape as part of functions into jsx

ghstack-source-id: a5df427981ca32406fb2325e583b64bbe26b1cdd
Pull Request resolved: #30714
Test Plan:
Fixes the previous issue: ref enforcement ignores memoization marker instructions

ghstack-source-id: f35d6a611c5e740e9ea354ec80c3d7cdb3c0d658
Pull Request resolved: #30715
…urrent

Summary:
Since we want to make ref-in-render errors enabled by default, we should position those errors at the location of the read. Not only will this be a better experience, but it also aligns the behavior of Forget and Flow.

This PR also cleans up the resulting error messages to not emit implementation details about place values.

ghstack-source-id: 1d1131706867a6fc88efddd631c4d16d2181e592
Pull Request resolved: #30723
Summary:
We previously were excessively strict about preventing functions that access refs from being returned--doing so is potentially valid for hooks, because the return value may only be used in an event or effect.

ghstack-source-id: cfa8bb1b54e8eb365f2de50d051bd09e09162d7b
Pull Request resolved: #30724
Summary:
The change earlier in this stack makes it less safe to have ref enforcement disabled. This diff enables it by default.

ghstack-source-id: d3ab5f1b28b7aed0f0d6d69547bb638a1e326b66
Pull Request resolved: #30716
)

enableHalt turns on a mode for flight prerenders where aborts are
treated like infinitely stalled outcomes while still completing the
prerender. For regular tasks we simply serialize the slot as a promise
that never settles. For ReadableStream, Blob, and Async Iterators we
just never advance the serialization so they remain unfinished when
consumed on the client.

When enableHalt is turned on aborts of prerenders will halt rather than
error. The abort reason is forwarded to the upstream produces of the
aforementioned async iterators, blobs, and ReadableStreams. In the
future if we expose a signal that you can consume from within a render
to cancel additional work the abort reason will also be forwarded there
This PR updates the babel plugin to continue the compilation pipeline as
normal on components/hooks that have been opted out using a directive.
Instead, we no longer emit the compiled function when the directive is
present.

Previously, we would skip over the entire pipeline. By continuing to
enter the pipeline, we'll be able to detect if there are unused
directives.

The end result is:

- (no change) 'use forget' will always opt into compilation
- (new) 'use no forget' will opt out of compilation but continue to log
  errors without throwing them. This means that a Program containing
multiple functions (some of which are opted out) will continue to
compile correctly

ghstack-source-id: 5bd85df2f81350cb2c1998a8761b8ed3fec32a40
Pull Request resolved: #30720
This PR updates the eslint plugin to report unused opt out directives.
One of the downsides of the opt out directive is that it opts the
component/hook out of compilation forever, even if the underlying issue
was fixed in product code or fixed in the compiler.

ghstack-source-id: 81deb5c11b7c57f07f6ab13266066cd73b2f3729
Pull Request resolved: #30721
This was a pet peeve where our playground could only compile top level
FunctionDeclarations. Just synthesize a fake identifier if it doesn't
have one.

ghstack-source-id: 882483c79ceebf382b69e37aed1f293efff9c5a7
Pull Request resolved: #30729
… ReactComponentInfo (#30717)

This enables finding Server Components on the owner path. Server
Components aren't stateful so there's not actually one specific owner
that it necessarily matches. So it can't be a global look up. E.g. the
same Server Component can be rendered in two places or even nested
inside each other.

Therefore we need to find an appropriate instance using a heuristic. We
can do that by traversing the parent path since the owner is likely also
a parent. Not always but almost always.

To simplify things we can also do the same for Fibers. That brings us
one step closer to being able to get rid of the global
fiberToFiberInstance map since we can just use the shadow tree to find
this information.

This does mean that we can't find owners that aren't parents which is
usually ok. However, there is a test case that's interesting where you
have a React ART tree inside a DOM tree. In that case the owners
actually span multiple renderers and roots so the owner is not on the
parent stack. Usually this is fine since you'd just care about the
owners within React ART but ideally we'd support this. However, I think
that really the fix to this is that the React ART tree itself should
actually show up inside the DOM tree in DevTools and in the virtual
shadow tree because that's conceptually where it belongs. That would
then solve this particular issue. We'd just need some way to associate
the root with a DOM parent when it gets mounted.
Addresses a todo from a while back. We now validate environment options when parsing the plugin options, which means we can stop re-parsing/validating in later phases.

ghstack-source-id: b19806e843e1254716705b33dcf86afb7223f6c7
Pull Request resolved: #30726
It is possible to throw after aborting during a render and we were not
properly tracking this. We use an AbortSigil to mark whether a rendering
task needs to abort but the throw interrupts that and we end up handling
an error on the error pathway instead.

This change reworks the abort-while-rendering support to be robust to
throws after calling abort
This uses a similar technique to what we use to generate fake stack
frames for server components. This generates an eval:ed wrapper function
around the Server Reference proxy we create on the client. This wrapper
function gets the original `name` of the action on the server and I also
add a source map if `findSourceMapURL` is defined that points back to
the source of the server function.

For `"use server"` on the server, there's no new API. It just uses the
callsite of `registerServerReference()` on the Server. We can infer the
function name from the actual function on the server and we already have
the `findSourceMapURL` on the client receiving it.

For `"use server"` imported from the client, there's two new options
added to `createServerReference()` (in addition to the optional
[`encodeFormAction`](#27563)). These are only used in DEV mode. The
[`findSourceMapURL`](#29708) option is the same one added in #29708. We
need to pass this these references aren't created in the context of any
specific request but globally. The other weird thing about this case is
that this is actually a case where the compiled environment is the
client so any source maps are the same as for the client layer, so the
environment name here is just `"Client"`.

```diff
  createServerReference(
    id: string,
    callServer: CallServerCallback,
    encodeFormAction?: EncodeFormActionCallback,
+   findSourceMapURL?: FindSourceMapURLCallback, // DEV-only
+   functionName?: string, // DEV-only
  )
```

The key is that we use the location of the
`registerServerReference()`/`createServerReference()` call as the
location of the function. A compiler can either emit those at the same
locations as the original functions or use source maps to have those
segments refer to the original location of the function (or in the case
of a re-export the original location of the re-export is also a fine
approximate). The compiled output must call these directly without a
wrapper function because the wrapper adds a stack frame. I decided
against complicated and fragile dev-only options to skip n number of
frames that would just end up in prod code. The implementation just
skips one frame - our own. Otherwise it'll just point all source mapping
to the wrapper.

We don't have a `"use server"` imported from the client implementation
in the reference implementation/fixture so it's a bit tricky to test
that. In the case of CJS on the server, we just use a runtime instead of
compiler so it's tricky to source map those appropriately. We can
implement it for ESM on the server which is the main thing we're testing
in the fixture. It's easier in a real implementation where all the
compilation is just one pass. It's a little tricky since we have to
parse and append to other source maps but I'd like to do that as a
follow up. Or maybe that's just an exercise for the reader.

You can right click an action and click "Go to Definition".

<img width="1323" alt="Screenshot 2024-08-17 at 6 04 27 PM"
src="https://github.com/user-attachments/assets/94d379b3-8871-4671-a20d-cbf9cfbc2c6e">

For now they simply don't point to the right place but you can still
jump to the right file in the fixture:

<img width="1512" alt="Screenshot 2024-08-17 at 5 58 40 PM"
src="https://github.com/user-attachments/assets/1ea5d665-e25a-44ca-9515-481dd3c5c2fe">

In Firefox/Safari given that the location doesn't exist in the source
map yet, the browser refuses to open the file. Where as Chrome does
nearest (last) line.
Per comments on the new validation pass, this disallows creating JSX (expression/fragment) within a try statement. Developers sometimes use this pattern thinking that they can catch errors during the rendering of the element, without realizing that rendering is lazy. The validation allows us to teach developers about the error boundary pattern.

ghstack-source-id: 0bc722aeaed426ddd40e075c008f0ff2576e0c33
Pull Request resolved: #30725
using infinitely suspending promises isn't right because this will parse
as a promise which is only appropriate if the value we're halting at is
a promise. Instead we need to have a special marker type that says this
reference will never resolve. Additionally flight client needs to not
error any halted references when the stream closes because they will
otherwise appear as an error

addresses:
#30705 (comment)
When printing these in DevTools they show up as the name of the
constructor so then you pass a Promise to the client it logs as "Chunk"
which is confusing.

Ideally we'd probably just name this Promise but 1) there's a slight
difference in the .then method atm 2) it's a bit tricky to name a
variable and get it from the global in the same scope. Closure compiler
doesn't let us just name a function because it removes it and just uses
the variable name.
Stacked on #30731.

When logging a Promise we emit it as an infinite promise instead of
blocking the replay on it.

This models that as a halted row instead. No need for this special case.

I unflag the receiving side since now it's used to replace a feature
that's already unflagged so it's used.
This reverts commit 52c9c43.

Just kidding. We realized we probably don't want to do the halted row
thing after all.
stacked on: #30731

We've refined the model of halting a prerender. Now when you abort
during a prerender we simply omit the rows that would complete the
flight render. This is analagous to prerendering in Fizz where you must
resume the prerender to actually result in errors propagating in the
postponed holes. We don't have a resume yet for flight and it's not
entirely clear how that will work however the key insight here is that
deciding whether the never resolving rows are an error or not should
really be done on the consuming side rather than in the producer.

This PR also reintroduces the logs for the abort error/postpone when
prerendering which will give you some indication that something wasn't
finished when the prerender was aborted.
Noticed this from #30707. This was vestigial from from circleci and now
that we're on GH actions I think we should be able to remove this option
altogether.

ghstack-source-id: 78e8b0243b1e1484ffaad820987ae3679a7374bf
Pull Request resolved: #30753
When aborting with a postpone value boundaries are put into client
rendered mode even during prerenders. This doesn't follow the postpoen
semantics of the rest of fizz where during a prerender a postpone is
tracked and it will leave holes in tracked postpone state that can be
resumed. This change updates this behavior to match the postpones
semantics between aborts and imperative postpones.
…ed missing chunk (#30750)

When aborting a prerender we should leave references unfulfilled, not
share a common unfullfilled reference. functionally today this doesn't
matter because we don't have resuming but the semantic is that the row
was not available when the abort happened and in a resume the row should
fill in. But by pointing each task to a common unfulfilled chunk we lose
the ability for these references to resolves to distinct values on
resume.
…nstead of erroring (#30732)

When we introduced prerendering for flight we modeled an abort of a
flight prerender as having unfinished rows. This is similar to how
postpone was already implemented when you postponed from "within" a
prerender using React.unstable_postpone. However when aborting with a
postponed instance every boundary would be eagerly marked for client
rendering which is more akin to prerendering and then resuming with an
aborted signal.

The insight with the flight work was that it's not so much the postpone
that describes the intended semantics but the abort combined with a
prerender. So like in flight when you abort a prerender and enableHalt
is enabled boundaries and the shell won't error for any reason. Fizz
will still call onPostpone and onError according to the abort reason but
the consuemr of the prerender should expect to resume it before trying
to use it.
Shortcut for the common case where only a single flag is checked. Same
as `gate(flags => flags.enableFeatureFlag)`.

Normally I don't care about these types of conveniences but I'm about to
add a lot more inline flag checks these all over our tests and it gets
noisy. This helps a bit.
In #29491 I updated the work
scheduler for Flight to use microtasks to perform work when something
pings. This is useful but it does have some downsides in terms of our
ability to do task prioritization. Additionally the initial work is not
instantiated using a microtask which is inconsistent with how pings
work.

In this change I update the scheduling logic to use microtasks
consistently for prerenders and use regular tasks for renders both for
the initial work and pings.
Follow up to #30741.

This is just for the reference Webpack implementation.

If there is a source map associated with a Node ESM loader, we generate
new source map entries for every `registerServerReference` call.

To avoid messing too much with it, this doesn't rewrite the original
mappings. It just reads them while finding each of the exports in the
original mappings. We need to read all since whatever we append at the
end is relative. Then we just generate new appended entries at the end.

For the location I picked the location of the local name identifier.
Since that's the name of the function and that gives us a source map
name index. It means it jumps to the name rather than the beginning of
the function declaration. It could be made more clever like finding a
local function definition if it is reexported. We could also point to
the line/column of the function declaration rather than the identifier
but point to the name index of the identifier name.

Now jumping to definition works in the fixture.

<img width="574" alt="Screenshot 2024-08-20 at 2 49 07 PM"
src="https://github.com/user-attachments/assets/7710f0e6-2cee-4aad-8d4c-ae985f8289eb">

Unfortunately this technique doesn't seem to work in Firefox nor Safari.
They don't apply the source map for jumping to the definition.
sebmarkbage and others added 29 commits September 30, 2024 15:45
The idea is that the RSC protocol is a superset of Structured Clone.
#25687 One exception that we left out was serializing Error objects as
values. We serialize "throws" or "rejections" as Error (regardless of
their type) but not Error values.

This fixes that by serializing `Error` objects. We don't include digest
in this case since we don't call `onError` and it's not really expected
that you'd log it on the server with some way to look it up.

In general this is not super useful outside throws. Especially since we
hide their values in prod. However, there is one case where it is quite
useful. When you replay console logs in DEV you might often log an Error
object within the scope of a Server Component. E.g. the default RSC
error handling just console.error and error object.

Before this would just be an empty object due to our lax console log
serialization:
<img width="1355" alt="Screenshot 2024-09-30 at 2 24 03 PM"
src="https://github.com/user-attachments/assets/694b3fd3-f95f-4863-9321-bcea3f5c5db4">
After:
<img width="1348" alt="Screenshot 2024-09-30 at 2 36 48 PM"
src="https://github.com/user-attachments/assets/834b129d-220d-43a2-a2f4-2eb06921747d">

TODO for a follow up: Flight Reply direction. This direction doesn't
actually serialize thrown errors because they always reject the
serialization.
This allows us to show props in React DevTools when inspecting a Server
Component.

I currently drastically limit the object depth that's serialized since
this is very implicit and you can have heavy objects on the server.

We previously was using the general outlineModel to outline
ReactComponentInfo but we weren't consistently using it everywhere which
could cause some bugs with the parsing when it got deduped on the
client. It also lead to the weird feature detect of `isReactComponent`.
It also meant that this serialization was using the plain serialization
instead of `renderConsoleValue` which means we couldn't safely serialize
arbitrary debug info that isn't serializable there.

So the main change here is to call `outlineComponentInfo` and have that
always write every "Server Component" instance as outlined and in a way
that lets its props be serialized using `renderConsoleValue`.

<img width="1150" alt="Screenshot 2024-10-01 at 1 25 05 AM"
src="https://github.com/user-attachments/assets/f6e7811d-51a3-46b9-bbe0-1b8276849ed4">
…esponse (#31102)

Fixes #31100.

There are 2 things:
1. In #30987, we've introduced a
breaking change: importing `react-devtools-core` is no longer enough for
installing React DevTools global Hook. You need to call `initialize`, in
which you may provide initial settings. I am not adding settings here,
because it is not implemented, and there are no plans for supporting
this.
2. Calling `installHook` is not necessary inside `standalone.js`,
because this script is running inside Electron wrapper (which is just a
UI, not the app that we are debugging). We will loose the ability to use
React DevTools on this React application, but I guess thats fine.
…st for tests (#31060)

Without this, the console gets spammy whenever we run React DevTools
tests against React 18.x, where this deprecation message was added.
We've dropped the support for detecting changes in legacy Contexts in
#30896.
We're seeing issues with this feature internally including bugs with
sibling prerendering and errors that are difficult for developers to
action on. We'll turn off the feature for the time being until we can
improve the stability and ergonomics.

This PR does two things:
- Turn off `enableInfiniteLoopDetection` everywhere while leaving it as
a variant on www so we can do further experimentation.
- Revert #31061 which was a
temporary change for debugging. This brings the feature back to
baseline.
Allow aborting encoding arguments to a Server Action if a Promise
doesn't resolve. That way at least part of the arguments can be used on
the receiving side. This leaves it unresolved in the stream rather than
encoding an error.

This should error on the receiving side when the stream closes but it
doesn't right now in the Edge/Browser versions because closing happens
immediately before we've had a chance to call `.then()` so the Chunks
are still in pending state. This is an existing bug also in
FlightClient.
## Summary

Creates a new `HostInstance` type for React Native, to more accurately
capture the intent most developers have when using the `NativeMethods`
type or `React.ElementRef<HostComponent<T>>`.

Since `React.ElementRef<HostComponent<T>>` is typed as
`React.AbstractComponent<T, NativeMethods>`, that means
`React.ElementRef<HostComponent<T>>` is equivalent to `NativeMethods`
which is equivalent to `HostInstance`.


## How did you test this change?

```
$ yarn
$ yarn flow fabric
```
Adds HIR version of `PropagateScopeDeps` to handle optional chaining.

Internally, this improves memoization on ~4% of compiled files (internal links: [1](https://www.internalfb.com/intern/paste/P1610406497/))

Summarizing the changes in this PR.
1. `CollectOptionalChainDependencies` recursively traverses optional blocks down to the base. From the base, we build up a set of `baseIdentifier.propertyA?.propertyB` mappings.
The tricky bit here is that optional blocks sometimes reference other optional blocks that are *not* part of the same chain e.g. a(c?.d)?.d. See code + comments in `traverseOptionalBlock` for how we avoid concatenating unrelated blocks.

2. Adding optional chains into non-null object calculation.
(Note that marking `a?.b` as 'non-null' means that `a?.b.c` is safe to evaluate, *not* `(a?.b).c`. Happy to rename this / reword comments accordingly if there's a better term)
This pass is split into two stages. (1) collecting non-null objects by block and (2) propagating non-null objects across blocks. The only significant change here was to (2). We add an extra reduce step `X=Reduce(Union(X, Intersect(X_neighbors)))` to merge optional and non-optional nodes (e.g. nonNulls=`{a, a?.b}` reduces to `{a, a.b}`)

3. Adding optional chains into dependency calculation.
This was the trickiest. We need to take the "maximal" property chain as a dependency. Prior to this PR, we avoided taking subpaths e.g. `a.b` of `a.b.c` as dependencies by only visiting non-PropertyLoad/LoadLocal instructions. This effectively only recorded the property-path at site-of-use.

    Unfortunately, this *quite* doesn't work for optional chains for a few reasons:
    - We would need to skip relevant `StoreLocal`/`Branch terminal` instructions (but only those within optional blocks that have been successfully read).
    - Given an optional chain, either (1) only a subpath or (2) the entire path can be represented as a PropertyLoad. We cannot directly add the last hoistable optional-block as a dependency as MethodCalls are an edge case e.g. given a?.b.c(), we should depend on `a?.b`, not `a?.b.c`
      This means that we add its dependency at either the innermost unhoistable optional-block or when encountering it within its phi-join.

4. Handle optional chains in DeriveMinimalDependenciesHIR.
This was also a bit tricky to formulate. Ideally, we would avoid a 2^3 case join (cond | uncond cfg, optional | not optional load, access | dependency). This PR attempts to simplify by building two trees
    1. First add each hoistable path into a tree containing `Optional | NonOptional` nodes.
    2. Then add each dependency into another tree containing `Optional | NonOptional`, `Access | Dependency` nodes, truncating the dependency at the earliest non-hoistable node (i.e. non-matching pair when walking the hoistable tree)

ghstack-source-id: a2170f26280dfbf65a4893d8a658f863a0fd0c88
Pull Request resolved: #31037
…ting (#31032)

Stack from [ghstack](https://github.com/ezyang/ghstack) (oldest at
bottom):
* #31066
* __->__ #31032

Prior to this PR, we check whether the property load source (e.g. the
evaluation of `<base>` in `<base>.property`) is mutable + scoped to
determine whether the property load itself is eligible for hoisting.
This changes to check the base identifier of the load.
- This is needed for the next PR #31066. We want to evaluate whether the
base identifier is mutable within the context of the *outermost
function*. This is because all LoadLocals and PropertyLoads within a
nested function declaration have mutable-ranges within the context of
the function, but the base identifier is a context variable.
- A side effect is that we no longer infer loads from props / other
function arguments as mutable in edge cases (e.g. props escaping out of
try-blocks or being assigned to context variables)
…n decls (#31066)

Stack from [ghstack](https://github.com/ezyang/ghstack) (oldest at
bottom):
* __->__ #31066
* #31032

Prior to this PR, we consider all of a nested function's accessed paths
as 'hoistable' (to the basic block in which the function was defined).
Now, we traverse nested functions and find all paths hoistable to their
*entry block*.

Note that this only replaces the *hoisting* part of function
declarations, not dependencies. This realistically only affects optional
chains within functions, which always get truncated to its inner
non-optional path (see
[todo-infer-function-uncond-optionals-hoisted.tsx](https://github.com/facebook/react/blob/576f3c0aa898cb99da1b7bf15317756e25c13708/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/todo-infer-function-uncond-optionals-hoisted.tsx))

See newly added test fixtures for details

Update: Note that toggling `enableTreatFunctionDepsAsConditional` makes
a non-trivial impact on granularity of inferred deps (i.e. we find that
function declarations uniquely identify some paths as hoistable).
Snapshot comparison of internal code shows ~2.5% of files get worse
dependencies ([internal
link](https://www.internalfb.com/phabricator/paste/view/P1625792186))
When we added support for Reanimated, we didn't distinguish between true
globals (i.e. identifiers with no static resolutions), module types, and
imports #29188. For the past 3-4 months, Reanimated imports were not
being matched to the correct hook / function shape we match globals and
module imports against two different registries.

This PR fixes our support for Reanimated library functions imported
under `react-native-reanimated`. See test fixtures for details
In order to support using the compiler on versions of React prior to 19,
we need the ability to statically import `c` (aka useMemoCache) or
fallback to a polyfill supplied by `react-compiler-runtime` (note: this
is a separate npm package, not to be confused with
`react/compiler-runtime`, which is currently a part of react).

To do this we first need to re-export `useMemoCache` under the top level
React namespace again, which is additive and thus non-breaking. Doing so
allows `react-compiler-runtime` to statically either re-export
`React.__COMPILER_RUNTIME.c` or supply a polyfill, without the need for
a dynamic import which is finicky to support due to returning a promise.

In later PRs I will remove `react/compiler-runtime` and update the
compiler to emit imports to `react-compiler-runtime` instead.
…1140)

This PR updates the standalone `react-compiler-runtime` package to
either re-export `React.__COMPILER_RUNTIME.c` or to use a userspace
polyfill.
We need `react-compiler-runtime` to use the same version of React as
snap
Updates the compiler to always import from `react-compiler-runtime` by
default. The runtime then decides whether to use the official or
userspace implementation of useMemoCache.
Now that the compiler always injects `react-compiler-runtime`, this
option is unnecessary.
Updates our publishing scripts to also publish react-compiler-runtime.
…e from backend (#31117)

We can't wait for a response from Backend, because it might take some
time to actually finish profiling.

We should keep a flag on the frontend side, so user can quickly see the
feedback in the UI.
Stacked on #31118. See last
commit.

We don't need to call `startProfiling()` here, because we delegate this
to the Renderer itself:

https://github.com/facebook/react/blob/830e823cd2c6ee675636d31320b10350e8ade9ae/packages/react-devtools-shared/src/backend/fiber/renderer.js#L5227-L5232

Since this is de-facto the constructor of Renderer, this will be called
earlier.

Validated via testing the reload-to-profile for Chrome browser
extension.
)

Based on #31049, credits to
@EdmondChuiHW.

What is happening here:
1. Once Agent is destroyed, unsubscribe own listeners and bridge
listeners.
2. [Browser extension only] Once Agent is destroyed, unsubscribe
listeners from BackendManager.
3. [Browser extension only] I've discovered that `backendManager.js`
content script can get injected multiple times by the browser. When
Frontend is initializing, it will create Store first, and then execute a
content script for bootstraping backend manager. If Frontend was
destroyed somewhere between these 2 steps, Backend won't be notified,
because it is not initialized yet, so it will not unsubscribe listeners
correctly. We might end up duplicating listeners, and the next time
Frontend is launched, it will report an issues "Cannot add / remove node
...", because same operations are emitted twice.

To reproduce 3 you can do the following:
1. Click reload-to-profile
2. Right after when both app and Chrome DevTools panel are reloaded,
close Chrome DevTools.
3. Open Chrome DevTools again, open Profiler panel and observe "Cannot
add / remove node ..." error in the UI.
Stacked on #31131. See last
commit.

This is a clean-up and a pre-requisite for next changes:
1. `ReloadAndProfileConfig` is now split into boolean value and settings
object. This is mainly because I will add one more setting soon, and
also because settings might be persisted for a longer time than the flag
which signals if the Backend was reloaded for profiling. Ideally, this
settings should probably be moved to the global Hook object, same as we
did for console patching.
2. Host is now responsible for reseting the cached values, Backend will
execute provided `onReloadAndProfileFlagsReset` callback.
Stacked on #31132. See last
commit.

There are 2 issues:
1. We've been recording timeline events, even if Timeline Profiler was
not supported by the Host. We've been doing this for React Native, for
example, which would significantly regress perf of recording a profiling
session, but we were not even using this data.
2. Currently, we are generating component stack for every state update
event. This is extremely expensive, and we should not be doing this.

We can't currently fix the second one, because we would still need to
generate all these stacks, and this would still take quite a lot of
time. As of right now, we can't generate a component stack lazily
without relying on the fact that reference to the Fiber is not stale.
With `enableOwnerStacks` we could populate component stacks in some
collection, which would be cached at the Backend, and then returned only
once Frontend asks for it. This approach also eliminates the need for
keeping a reference to a Fiber.
@pull pull bot merged commit d5bba18 into reanimatedmanx:main Oct 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.