Mirror of upstream PR #34392 #386
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…listeners (facebook#33743) This is the same as we do for currently rendering tasks. They get effectively sync aborted when the listener is invoked. We potentially miss out on some debug info in that case but that would only apply to any entries inside the stream which doesn't really have their own debug info anyway.
In playground it's helpful to show all errors, even those that don't completely abort compilation. For example, to help demonstrate that the compiler catches things like setState in effects. This detects these errors and ensures we show them.
…ebook#33746) * Error for `eval()` * More specific error message for `with (expr) { ... }` syntax * More specific error message for class declarations --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33746). * facebook#33752 * facebook#33751 * facebook#33750 * facebook#33748 * facebook#33747 * __->__ facebook#33746
…k#33747) Supports inline enum declarations in both Flow and TS by treating the node as pass-through (enums can't capture values mutably). Related, this PR extends the set of type-related declarations that we ignore. Previously we threw a todo for things like DeclareClass or DeclareVariable, but these are type related and can simply be dropped just like we dropped TypeAlias. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33747). * facebook#33753 * facebook#33752 * facebook#33751 * facebook#33750 * facebook#33748 * __->__ facebook#33747
…atements (facebook#33748) import, export, and TS namespace statements can only be used at the top-level of a module, which is enforced by parsers already. Here we add a backup validation of that. As of this PR, we now have only major statement type (class declarations) listed as a todo. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33748). * facebook#33753 * facebook#33752 * facebook#33751 * facebook#33750 * __->__ facebook#33748
We use this variant for syntax we intentionally don't support: with statements, eval, and inline class declarations. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33750). * facebook#33753 * facebook#33752 * facebook#33751 * __->__ facebook#33750 * facebook#33748
…ook#33755) When postponing the root we encode the segment Id into the postponed state but we should really be reseting it to zero so we can restart the counter from the beginning when the resume is actually just a re-render. This also no longer assigns the root segment id based on the postponed state when resuming the root for the same reason. In the future we may use the embedded replay segment id if we implement resuming the root without re-rendering everything but that is not yet implemented or planned.
We typically treat an empty message as closing the debug channel stream but for the Noop renderer we don't use an intermediate stream but just pass the message through. https://github.com/facebook/react/blob/bbc13fa17be8eebef3e6ee47f48c76c0c44e2f36/packages/react-server-dom-webpack/src/client/ReactFlightDOMClientBrowser.js#L59-L60 For that simple case we should just treat it as a close without an intermediate stream.
This lets us pass a writable on the server side and readable on the client side to send debug info through a separate channel so that it doesn't interfere with the main payload as much. The main payload refers to chunks defined in the debug info which means it's still blocked on it though. This ensures that the debug data has loaded by the time the value is rendered so that the next step can forward the data. This will be a bit fragile to race conditions until facebook#33665 lands. Another follow up needed is the ability to skip the debug channel on the receiving side. Right now it'll block forever if you don't provide one since we're blocking on the debug data.
…osures (facebook#33544) Summary: useEffectEvent is meant to be used specifically in combination with useEffect, and using the feature in arbitrary closures can lead to surprising reactivity semantics. In order to minimize risk in the experimental rollout, we are going to restrict its usage to being called directly inside an effect or another useEffectEvent, effectively enforcing the function coloring statically. Without an effect system this is the best we can do.
…33757) React Elements reference debug data (their stack and owner) in the debug channel. If the debug channel isn't wired up this can block the client from resolving. We can infer that if there's no debug channel wired up and the reference wasn't emitted before the element, then it's probably because it's in the debug channel. So we can skip it. This should also apply to debug chunks but they're not yet blocking until facebook#33665 lands.
facebook#33788) ## Summary The `TSAsExpression` and `TSNonNullExpression` nodes are supported by `lowerExpression()` but `isReorderableExpression()` does not check if they can be reordered. This PR updates `isReorderableExpression()` to handle these two node types by adding cases that fall through to the existing `TypeCastExpression` case. We ran `react-compiler-healthcheck` at scale on several of our repos and found dozens of `` (BuildHIR::node.lowerReorderableExpression) Expression type `TSAsExpression` cannot be safely reordered`` errors and a handful for `TSNonNullExpression`. ## How did you test this change? In this case I added two fixture tests
…acebook#33797) This fixes displaying incorrect component render entries on a timeline, when we are reconnecting passive effects. ### Before <img width="2318" height="1127" alt="1" src="https://github.com/user-attachments/assets/9b6b2824-d2de-43a3-8615-2c45d67c3668" /> The cloned nodes will persist original `actualStartTime`, when these were first mounted. When we "replay", the end time will be "now" or whatever the actual start time of the sibling. Depending on when this is being recorded, the diff between end and start could be tens of seconds and doesn't represent what React was doing. We shouldn't log these entries at all. ### After We are only logging newly finished renders, but could potentially loose renders that never commit.
…it Promises (facebook#33798) We already do this with `"new Promise"` and `"Promise.then"`. There are also many helpers that both create promises and awaits other promises inside of it like `Promise.all`. The way this is filtered is different from just filtering out all anonymous stacks since they're used to determine where the boundary is between ignore listed and user space. Ideally we'd cover more wrappers that are internal to Promise libraries.
--- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33799). * facebook#33800 * __->__ facebook#33799
…facebook#33665) This resolves an outstanding issue where it was possible for debug info and console logs to become out of order if they up blocked. E.g. by a future reference or a client reference that hasn't loaded yet. Such as if you console.log a client reference followed by one that doesn't. This encodes the order similar to how the stream chunks work. This also blocks the main chunk from resolving until the last debug info has fully loaded, including future references and client references. This also ensures that we could send some of that data in a different stream, since then it can come out of order.
…k#33943) ## Summary We have thoroughly tested this flag in production and proved stability and performance, so we can clean it up and "ship it".
…acebook#33947) If you pass a promise to a client component to be rendered `<Client promise={promise} />` then there's an internal await inside Flight. There might also be user space awaits but those awaits may already have happened before we render this component. Conceptually they were part of the parent component and not this component. It's tricky to attribute which await should be used for the stack in this case. If we can't find an await we can use the JSX callsite as the stack frame. However, we don't want to do this for simple cases like if you return a non-native Promise from a Server Component. Since that would now use the stack of the thing that rendered the Server Component which is worse than the stack of the I/O. To fix this, I update the `debugOwner`/`debugTask`/`debugStack` when we start rendering inside the Server Component. Conceptually these represent the "parent" component and is used for errors referring to the parent like when we serialize client component props the parent is the JSX of the client component. However, when we're directly inside the Server Component we don't have a callsite of the parent really. Conceptually it would be the return call of the Server Component. This might negatively affect other types of errors but I think this is ok since this feature mainly exists for the case when you enter the child JSX.
…acebook#33948) We need a "value" to represent the I/O that was loaded. We don't normally actually use the Promise at the callsite that started the I/O because that's usually deep inside internals. Instead we override the value of the I/O entry with the Promise that was first awaited in user space. This means that you could potentially have different values depending on if multiple things await the same I/O. We just take one of them. (Maybe we should actually just write the first user space awaited Promise as the I/O entry? This might instead have other implications like less deduping.) When you pass a Promise forward, we may skip the awaits that happened in earlier components because they're not part of the currently rendering component. That's mainly for the stack and time stamps though. The value is still probably conceptually the best value because it represents the I/O value as far user space is concerned. This writes the I/O early with the first await we find in user space even if we're not going to use that particular await for the stack.
This makes it so you can click the source location itself to view the source. This is similar styling as the link to jump to function props like events and actions. We're going to need a lot more linkifying to jump to various source locations. Also, I always was trying to click this file anyway. Hover state: <img width="485" height="382" alt="Screenshot 2025-07-21 at 4 36 10 PM" src="https://github.com/user-attachments/assets/1f0f8f8c-6866-4e62-ab84-1fb5ba012986" />
facebook#33955) In RSC and other stacks now we use a lot of `ReactFunctionLocation` type to represent the location of a function. I.e. the location of the beginning of the function (the enclosing line/col) that is represented by the "Source" of the function. This is also what the parent Component Stacks represents. As opposed to `ReactCallSite` which is what normal stack traces and owner stacks represent. I.e. the line/column number of the callsite into the next function. We can start sharing more code by using the `ReactFunctionLocation` type to represent the component source location and it also helps clarify which ones are function locations and which ones are callsites as we start adding more stack traces (e.g. for async debug info and owner stack traces).
If a `file:///` path is specified as the url of a file, like after source mapping into an ESM file, then we should be able to open it in a code editor.
…acebook#33968) This adds a "Code Editor" pane for the Chrome extension in the bottom right corner of the "Sources" panel. If you end up getting linked to the "Sources" panel from stack traces in console, performance tab, stacks in React Component tab like the one added in facebook#33954 basically everywhere there's a link to source code. Then going from there to open in a code editor should be more convenient. This adds a button to open the current file. <img width="1387" height="389" alt="Screenshot 2025-07-22 at 10 22 19 PM" src="https://github.com/user-attachments/assets/fe01f84c-83c2-4639-9b64-4af1a90c3f7d" /> This only makes sense in the extensions since in standalone it needs to always open by default in an editor. Unfortunately Firefox doesn't support extending the Sources panel. Chrome is also a bit buggy where it doesn't send a selection update event when you switch tabs in the Sources panel. Only when the actual cursor position changes. This means that the link can be lagging behind sometimes. We also have some general bugs where if React DevTools loses connection it can break the UI which includes this pane too. This has a small inline configuration too so that it's discoverable: <img width="559" height="143" alt="Screenshot 2025-07-22 at 10 22 42 PM" src="https://github.com/user-attachments/assets/1270bda8-ce10-4f9d-9fcb-080c0198366a" /> <img width="527" height="123" alt="Screenshot 2025-07-22 at 10 22 30 PM" src="https://github.com/user-attachments/assets/45848c95-afd8-495f-a7cf-eb2f46e698f2" /> Since we can't add a separate link to open-in-editor or open-in-sources everywhere I plan on adding an option to open in editor by default in a follow up. That option needs to be even more discoverable. I moved the configuration from the Components settings to the General settings since this is now a much more general features for opening links to resources in all types of panes. <img width="673" height="311" alt="Screenshot 2025-07-22 at 10 22 57 PM" src="https://github.com/user-attachments/assets/ea2c0871-942c-4b55-a362-025835d2c2bd" />
<!-- Thanks for submitting a pull request! We appreciate you spending the time to work on these changes. Please provide enough information so that others can review your pull request. The three fields below are mandatory. Before submitting a pull request, please make sure the following is done: 1. Fork [the repository](https://github.com/facebook/react) and create your branch from `main`. 2. Run `yarn` in the repository root. 3. If you've fixed a bug or added code that should be tested, add tests! 4. Ensure the test suite passes (`yarn test`). Tip: `yarn test --watch TestName` is helpful in development. 5. Run `yarn test --prod` to test in the production environment. It supports the same options as `yarn test`. 6. If you need a debugger, run `yarn test --debug --watch TestName`, open `chrome://inspect`, and press "Inspect". 7. Format your code with [prettier](https://github.com/prettier/prettier) (`yarn prettier`). 8. Make sure your code lints (`yarn lint`). Tip: `yarn linc` to only check changed files. 9. Run the [Flow](https://flowtype.org/) type checks (`yarn flow`). 10. If you haven't already, complete the CLA. Learn more about contributing: https://reactjs.org/docs/how-to-contribute.html --> ## Summary Part 2 of adding a "Config Override" panel to the React compiler playground. Added sync from the config editor (still only accessible with the "showConfig" param) to the main source code editor. Adding a valid config to the editor will add/replace the `@OVERRIDE` pragma above the source code. Additionally refactored the old implementation to remove `useEffect`s and unnecessary renders. Realized upon testing that the user experience is quite jarring, planning to add a `sync` button in the next PR to fix this. ## How did you test this change? <!-- Demonstrate the code is solid. Example: The exact commands you ran and their output, screenshots / videos if the pull request changes the user interface. How exactly did you verify that your PR solves the issue you wanted to solve? If you leave this empty, your PR will very likely be closed. --> https://github.com/user-attachments/assets/a71b1b5f-0539-4c00-8d5c-22426f0280f9
…ole task available (facebook#34370) React Native doesn't support `console.createTask` yet, but it does support `performance.measure` and extensibility APIs for Performance panel, including `detail.devtools` field. Previously, this logic was gated with `if (__DEV__ && debugTask)`, now `debugTask` is no longer required to log render. If there is no console task, we will just call `performance.measure(...)`. The same pattern is used in other reporters.
…34373) So we can test the feature.
We were still on a canary version of next in the playground, so let's update to the latest version.
…ook#34376) Fixes a bug in useDeferredValue's optional `initialValue` argument. In the regression case, if a new useDeferredValue hook is mounted while an earlier transition is suspended, the `initialValue` argument of the new hook was ignored. After the fix, the `initialValue` argument is correctly rendered during the initial mount, regardless of whether other transitions were suspended. The culprit was related to the mechanism we use to track whether a render is the result of a `useDeferredValue` hook: we assign the deferred lane a TransitionLane, then entangle that lane with the DeferredLane bit. During the subsequent render, we check for the presence of the DeferredLane bit to determine whether to switch to the final, canonical value. But because transition lanes can themselves become entangled with other transitions, the effect is that every entangled transition was being treated as if it were the result of a `useDeferredValue` hook, causing us to skip the initial value and go straight to the final one. The fix I've chosen is to reserve some subset of TransitionLanes to be used only for deferred work, instead of using entanglement. This is similar to how retries are already implemented. Originally I tried not to implement it this way because it means there are now slightly fewer lanes allocated for regular transitions, but I underestimated how similar deferred work is to retries; they end up having a lot of the same requirements. Eventually it may be possible to merge the two concepts.
Fixes facebook#34108. If a scope ends with with a conditional where some/all branches exit via labeled break, we currently compile in a way that works but bypasses memoization. We end up with a shape like ```js let t0; label: { if (changed) { ... if (cond) { t0 = ...; break label; } // we don't save the output if the break happens! t0 = ...; $[0] = t0; } else { t0 = $[0]; } ``` The fix here is to update AlignReactiveScopesToBlockScopes to take account of breaks that don't go to the natural fallthrough. In this case, we take any active scopes and extend them to start at least as early as the label, and extend at least to the label fallthrough. Thus we produce the correct: ```js let t0; if (changed) { label: { ... if (cond) { t0 = ...; break label; } t0 = ...; } // now the break jumps here, and we cache the value $[0] = t0; } else { t0 = $[0]; } ``` --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34335). * facebook#34347 * facebook#34346 * facebook#34343 * __->__ facebook#34335
…nctions (facebook#34343) `@enablePreserveExistingMemoizationGuarantees` mode currently does not guarantee memoization of primitive-returning functions. We're often able to infer that a function returns a primitive based on how its result is used, for example `foo() + 1` or `object[getIndex()]`, and by default we do not currently memoize computation that produces a primitive. The reasoning behind this is that the compiler is primarily focused on stopping cascading updates — it's fine to recompute a primitive since we can cheaply compare that primitive and avoid unnecessary downstream recomputation. But we've gotten a lot of feedback that people find this surprising, and that sometimes the computation can be expensive enough that it should be memoized. This PR changes `@enablePreserveExistingMemoizationGuarantees` mode to ensure that primitive-returning functions get memoized. Other modes will not memoize these functions. Separately from this we are considering enabling this mode by default. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34343). * facebook#34347 * facebook#34346 * __->__ facebook#34343 * facebook#34335
…acebook#34346) I tried turning on `@enablePreserveExistingMemoizationGuarantees` by default and cleaned up a couple small things: * We emit freeze calls for StartMemoize deps but these had ValueReason.Other so the message wasn't great. We now treat these like other hook arguments. * PruneNonEscapingScopes was being too aggressive in this mode and memoizing even loads of globals. Switching to MemoizationLevel.Conditional ensures we build a graph that connects through to primitive-returning function calls, but doesn't unnecessarily force memoization otherwise. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34346). * facebook#34347 * __->__ facebook#34346
kushxg
force-pushed
the
upstream-pr-34392
branch
2 times, most recently
from
43c5c68 to
4745244
Compare
Adds some new test cases for ValidateNoDerivedComputationsInEffects.
…ps derived effects This PR adds infra to disambiguate between two types of derived state in effects: 1. State derived from props 2. State derived from other state TODO: - [ ] Props tracking through destructuring and property access does not seem to be propagated correctly inside of Functions' instructions (or i might be misunderstanding how we track aliasing effects) - [ ] compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/invalid-derived-state-from-props-computed.js should be failing - [ ] Handle "mixed" case where deps flow from at least one prop AND state. Should probably have a different error reason, to aid with categorization
…ct is used elsewhere
…o derived computations in effects
kushxg
force-pushed
the
upstream-pr-34392
branch
3 times, most recently
from
c94d769 to
a46e4b8
Compare
kushxg
force-pushed
the
upstream-pr-34392
branch
from
a46e4b8 to
e380d2e
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Mirrored from facebook/react PR facebook#34392