A whole bunch of changes to snap aimed at making it more usable for humans and agents. Here's the new CLI interface:
```
node dist/main.js --help
Options:
--version Show version number [boolean]
--sync Run compiler in main thread (instead of using worker
threads or subprocesses). Defaults to false.
[boolean] [default: false]
--worker-threads Run compiler in worker threads (instead of
subprocesses). Defaults to true.
[boolean] [default: true]
--help Show help [boolean]
-w, --watch Run compiler in watch mode, re-running after changes
[boolean]
-u, --update Update fixtures [boolean]
-p, --pattern Optional glob pattern to filter fixtures (e.g.,
"error.*", "use-memo") [string]
-d, --debug Enable debug logging to print HIR for each pass[boolean]
```
Key changes:
* Added abbreviations for common arguments
* No more testfilter.txt! Filtering/debugging works more like Jest, see below.
* The `--debug` flag (`-d`) controls whether to emit debug information. In watch mode, this flag sets the initial debug value, and it can be toggled by pressing the 'd' key while watching.
* The `--pattern` flag (`-p`) sets a filter pattern. In watch mode, this flag sets the initial filter. It can be changed by pressing 'p' and typing a new pattern, or pressing 'a' to switch to running all tests.
* As before, we only actually enable debugging if debug mode is enabled _and_ there is only one test selected.
Initializes CLAUDE.md and a settings file for the compiler/ directory to help use claude with the compiler. Note that some of the commands here depend on changes to snap from the next PR.
# Summary
note: This implements the idea discussed in https://github.com/reactwg/react/discussions/389#discussioncomment-14252280
This is a large PR that significantly changes our impurity and ref validation to address multiple issues. The goal is to reduce false positives and make the errors we do report more actionable.
## Validating Against Impure Values In Render
Currently we create `Impure` effects for impure functions like `Date.now()` or `Math.random()`, and then throw if the effect is reachable during render. However, impurity is a property of the resulting value: if the value isn't accessed during render then it's okay: maybe you're console-logging the time while debugging (fine), or storing the impure value into a ref and only accessing it in an effect or event handler (totally ok).
This PR updates to validate that impure values are not transitively consumed during render, building on the new effects system: rather than look at instruction types, we use effects like `Capture a -> b`, `Impure a`, and `Render b` to determine how impure values are introduced and where they flow through the program. We're intentionally conservative, and do _not_ propagate impurity through MaybeCapture effects, which is used for functions we don't have signatures for. This means that things like `identity(performance.now())` drop the impurity. This feels like a good compromise since it means we have very high confidence in the errors that we report and can always add increased strictness later as our confidence increases.
An example error:
```
Error: Cannot access impure value during render
Calling an impure function can produce unstable results that update unpredictably when the component happens to re-render. (https://react.dev/reference/rules/components-and-hooks-must-be-pure#components-and-hooks-must-be-idempotent).
error.invalid-impure-value-in-render-helper.ts:5:17
3 | const now = () => Date.now();
4 | const render = () => {
> 5 | return <div>{now()}</div>;
| ^^^^^ Cannot access impure value during render
6 | };
7 | return <div>{render()}</div>;
8 | }
error.invalid-impure-value-in-render-helper.ts:3:20
1 | // @validateNoImpureFunctionsInRender
2 | function Component() {
> 3 | const now = () => Date.now();
| ^^^^^^^^^^ `Date.now` is an impure function.
4 | const render = () => {
5 | return <div>{now()}</div>;
6 | };
```
Impure values are allowed to flow into refs, meaning that we now allow `useRef(Date.now())` or `useRef(localFunctionThatReturnsMathDotRandom())` which would have errored previously. The next PR reuses this improved impurity tracking to validate ref access in render as well.
## Refs Now Treated As Impure Values in Render
Reading a ref now produces an `Impure` effect, and reading refs in render is validated using the above validation against impure values in render. The error category and message is customized for refs, we're just reusing the validation implementation. This means you get consistent results for `performance.now()` as for `ref.current`. A nice consistency win.
## Simplified writing-ref validation
Now that _reading_ a ref in render is validated using the impure values infra, I also dramatically simplified ValidateNoRefAccessInRender to focus solely on validation against _writing_ refs during render. It was harder to use the new effects infra for this since we intentionally do not record ref mutations as a `Mutate` effect. So for now, the pass switches on InstructionValue variants. We continue to support the `if (ref.current == null) { ref.current = <init> }` pattern and reasonable variants of it. We're conservative about what we consider to be a write of a ref - `foo(ref)` now assumes you're not mutating rather than the inverse.
# Takeaways
* Impure-values-in-render logic is more conservative about what it considers an error (ie to users it appears more persmissive). We follow clearly identifiable (and if we wanted traceable/explainable) paths from impure sources through to where they are rendered. We allow many more cases than before, notably `x = foo(ref)` optimistically assumes you don't read the ref. Concretely, this follows from not tracking impure values across MaybeCapture effects.
* No-writing-refs-in-render is also more conservative about what it counts as a write, appearing to users as allowing more cases. We look for very direct evidence of writing to refs, ie `ref.current = <value>` or `ref.current.property = <value>`, and allow potential writes through eg `writeToRef(ref, value)`.
Jest's default test sequencer sorts alphabetically, causing large test
files
(eg ReactDOMFloat-test.js at 9k lines,
ReactHooksWithNoopRenderer-test.js at 4k
lines) to cluster in shard 3/5. This made shard 3/5 average 117s vs 77s
for
other shards, a 52% slowdown. I'm using filesize as a rough proxy for
number of tests.
This custom sequencer sorts tests by file size and distributes large
files evenly across all shards
instead of clustering them together.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/35458).
* __->__ #35458
* #35459
DevTools has ~45 test files which don't distribute well across 10
shards,
causing shard 3 to run 2x slower than others (104s vs ~50s). This moves
DevTools build tests to a separate job with 3 shards for better load
balancing.
When the Fizz runtime runs a view-transition we apply
`view-transition-name` and `view-transition-class` to the `style`. These
can be observed by Fiber when hydrating which incorrectly leads to
hydration errors.
More over, even after we remove them, the `style` attribute has now been
normalized which we are unable to diff because we diff against the SSR
generated `style` attribute string and not the normalized form. So if
there are other inline styles defined, we have to skip diffing them in
this scenario.
## Summary
This PR improves cyclic thenable detection in
`ReactFlightReplyServer.js`. Fixes#35368.
The previous fix only detected direct self-references (`inspectedValue
=== chunk`) and relied on the `cycleProtection` counter to eventually
bail out of longer cycles. This change keeps the existing
MAX_THENABLE_CYCLE_DEPTH ($1000$) `cycleProtection` cap as a hard
guardrail and adds a visited set so that we can detect self-cycles and
multi-node cycles as soon as any `ReactPromise` is revisited and while
still bounding the amount of work we do for deep acyclic chains via
`cycleProtection`.
## How did you test this change?
- Ran the existing test suite for the server renderer:
```bash
yarn test react-server
yarn test --prod react-server
yarn flow dom-node
yarn linc
```
---------
Co-authored-by: Hendrik Liebau <mail@hendrik-liebau.de>
`react-hooks/exhaustive-effect-dependencies` from
`ValidateExhaustiveDeps` reports errors for both missing and extra
effect deps. We already have `react-hooks/exhaustive-deps` that errors
on missing dependencies. In the future we'd like to consolidate this all
to the compiler based error, but for now there's a lot of overlap. Let's
enable testing the extra dep warning by splitting out reporting modes.
This PR
- Creates `on`, `off`, `missing-only`, and `extra-only` reporting modes
for the effect dep validation flag
- Temporarily enables the new rule with `extra-only` in
`eslint-plugin-react-hooks`
- Adds additional null checking to `manualMemoLoc` to fix a bug found
when running against the fixture
Summary:
These validations are not essential for compilation, with this we only
run that logic when outputMode is 'lint'
Test Plan:
Update fixtures and run tests
Putting up https://github.com/facebook/react/pull/35129 again
Reverted in https://github.com/facebook/react/pull/35346 after breaking
main before security patch
This change impacts output formatting in a lot of snaps, so is very
sensitive to additions in main to the fixtures resulting in broken tests
after merging, so we should try merge quickly after rebasing or do a
fast follow to the merge with a snap update.
Server Functions can be stringified (sometimes implicitly) when passed
as data. This adds an override to hide the source code in that case -
just in case someone puts sensitive information in there.
Note that this still preserves the `name` field but this is also
available on the export but in practice is likely minified anyway.
There's nothing else on these referenes we'd consider unsafe unless you
explicitly expose expandos which are part of the `"use server"` export.
This adds a safety check to ensure you don't encode cyclic Promises.
This isn't a parser bug per se. Promises do have a safety mechanism that
avoids them infinite looping. However, since we use custom Thenables,
what can happen is that every time a native Promise awaits it, another
Promise wrapper is created around the Thenable which foils the
ECMAScript Promise cycle detection which can lead to an infinite loop.
This also ensures that embedded `ReadableStream` and `AsyncIterable`
streams are properly closed if the source stream closes early both on
the Server and Client. This doesn't cause an infinite loop but just to
make sure resource clean up can proceed properly.
We're also adding some more explicit clear errors for invalid payloads
since we no longer need to obfuscate the original issue.
### What
Fixes source locations for VariableDeclarator in the generated AST.
Fixes a number of the errors in the snapshot I added yesterday in the
source loc validator PR https://github.com/facebook/react/pull/35109
I'm not entirely sure why, but a side effect of the fix has resulted in
a ton of snaps needing updating, with some empty lines no longer present
in the generated output. I broke the change up into 2 separate commits.
The [first
commit](f4e4dc0f44)
has the core change and the update to the missing source locations test
expectation, and the [second
commit](cd4d9e944c)
has the rest of the snapshot updates.
### How
- Add location for variable declarators in ast codegen.
- We don't actually have the location preserved in HIR, since when we
lower the declarations we pass through the location for the
VariableDeclaration. Since VariableDeclarator is just a container for
each of the assignments, the start of the `id` and end of the `init` can
be used to accurately reconstruct it when generating the AST.
- Add source locations for object/array patterns for destructuring
assignment source location support
`Error.prepareStackTrace` is non-standard feature and not all JavaScript
runtimes implement the methods that we are using in React DevTools
backend.
This PR adds additional checks for the presence of the methods that we
are using.
Follow-up to https://github.com/facebook/react/pull/34653.
React Native doesn't implement `getClientRect`, since this is applicable
to CSS box, which is not a concept for Native (maybe yet).
I am loosening the condition that gates `showOverlay()` call to pass if
`getClientRect` is not implemented.
Conceptually, everything that is inside `react-devtools-shared/backend`
should be Host-agnostic, because both on Web and Native it is installed
inside the Host JavaScript runtime, be it main frame of the page, or RN
instance. Since overlay & highlighting logic also lives there, it should
also follow these principles.
Continue attaching `internalInstanceKey` to DOM nodes in DEV. This
prevents breaking some internal dev tooling while we experiment with the
broader change. Note that this does not reference the DOM handle within
the flag, just attaches it and deletes it. Internals tracking is still
done through the private map.
## Summary
Add keyboard shortcuts (Cmd/Ctrl + Left/Right arrow keys) to navigate
between commits in the Profiler's snapshot view.
Moved `filteredCommitIndices` management and commit navigation logic
(`selectNextCommitIndex`, `selectPrevCommitIndex`) from
`SnapshotSelector` into `useCommitFilteringAndNavigation` used by
`ProfilerContext` to enable keyboard shortcuts from the top-level
Profiler component.
## How did you test this change?
- New tests in ProfilerContext-tests
- Built browser extension: `yarn build:<browser name>`
- tested in browser: `yarn run test:<browser name>`
- Manually verified Left/Right arrow navigation cycles through commits
- Verified navigation respects commit duration filter
- Verified reload-and-profile button unaffected
Chrome:
https://github.com/user-attachments/assets/01d2a749-13dc-4d08-8bcb-3d4d45a5f97c
Edge with duration filter:
https://github.com/user-attachments/assets/a7f76ff7-2a0b-4b9c-a0ce-d4449373308b
firefox mixing hotkey with clicking arrow buttons:
https://github.com/user-attachments/assets/48912d68-7c75-40f2-a203-5e6d7e6b2d99
Speculative fix to https://github.com/facebook/react/issues/35336
written by Claude.
I have verified that applying a similar patch locally to the repro from
#35336 does fix the crash.
I'm not familiar enough with the underlying APIs to tell whether the fix
is correct or sufficient.
Was bumped to a canary in https://github.com/facebook/react/pull/34499/
which got never released as stable.
Presumeably to use `Activity` which only made it into Activity in later
Next.js releases. However, `Activity` never ended up being used due to
incompatibilities with Monaco Editor. Downgrading should be safe.
Downgrading to fix
https://github.com/vercel/next.js/security/advisories/GHSA-9qr9-h5gf-34mp.
This will allow new deploys since Vercel is currently blocking new
deploys of unsafe version
---------
Co-authored-by: Eugene Choi <4eugenechoi@gmail.com>
The current `validateNoSetStateInEffects` error has potential false
positives because
we cannot fully statically detect patterns where calling setState in an
effect is
actually valid. This flag `enableVerboseNoSetStateInEffect` adds a
verbose error mode that presents multiple possible
use-cases, allowing an agent to reason about which fix is appropriate
before acting:
1. Non-local derived data - suggests restructuring state ownership
2. Derived event pattern - suggests requesting an event callback from
parent
3. Force update / external sync - suggests using `useSyncExternalStore`
This gives agents the context needed to make informed decisions rather
than
blindly applying a fix that may not be correct for the specific
situation.
Alternative approach to #35282 for validating effect deps in the
compiler that builds on the machinery in ValidateExhaustiveDependencies.
Key changes to that pass:
* Refactor to track the dependencies of array expressions as temporaries
so we can look them up later if they appear as effect deps.
* Instead of not storing temporaries for LoadLocals of locally created
variables, we store the temporary but also propagate the local-ness
through. This allows us to record deps at the top level, necessary for
effect deps. Previously the pass was only ever concerned with tracking
deps within function expressions.
* Refactor the bulk of the dependency-checking logic from
`onFinishMemoize()` into a standalone helper to use it for the new
`onEffect()` helper as well.
* Add a new ErrorCategory for effect deps, use it for errors on
effects
* Put the effect dep validation behind a feature flag
* Adjust the error reason for effect errors
---------
Co-authored-by: Jack Pope <jackpope1@gmail.com>
Fixes an edge case where a function expression would fail to take a
dependency if it referenced a hoisted `const` inferred as a primitive
value. We were incorrectly skipping primitve-typed operands when
determing scopes for merging in InferReactiveScopeVariables.
This was super tricky to debug, for posterity the trick is that Context
variables (StoreContext etc) are modeled just like a mutable object,
where assignment to the variable is equivalent to `object.value = ...`
and reading the variable is equivalent to `object.value` property
access. Comparing to an equivalent version of the repro case replaced
with an object and property read/writes showed that everything was
exactly right, except that InferReactiveScopeVariables wasn't merging
the scopes of the function and the context variable, which led me right
to the problematic line.
Closes#35122
Follow-up to https://github.com/facebook/react/pull/34641.
Similar to https://github.com/facebook/react/pull/35293,
https://github.com/facebook/react/pull/35294.
React DevTools backend can be used in non-DOM environments, so we have
to feature-check some DOM APIs.
For now I am just no-oping newly added commands for Native, we should
revisit this decision once we would roll out Suspense panel there, if
needed. I am not sure if scrolling will be required as much as it is
needed on Web.
`isReactNativeEnvironment()` check is kinda clowny, but we've been
relying on it for quite some time already.
AFAIK this is not needed to prevent any exploit but we don't really need
this. We allow functions on pretty much any other object anyway but
never on the "then" property since those would be serialized as Promises
by the client anyway.
Adds a new `enableUseKeyedState` compiler flag that changes the error
message for unconditional setState calls during render.
When `enableUseKeyedState` is enabled, the error recommends using
`useKeyedState(initialState, key)` to reset state when dependencies
change. When disabled (the default), it links to the React docs for the
manual pattern of storing previous values in state.
Both error messages now include helpful bullet points explaining the two
main alternatives:
1. Use useKeyedState (or manual pattern) to reset state when other
state/props change
2. Compute derived data directly during render without using state
FlightReplyServer are for client->server and ReactFlightClient is for
server->client. They're not 100% symmetrical.
We did a number of refactors to ReactFlightClient in PRs like #29823 and
#33664 to change the structure of the resolution. This PR brings those
changes to synchronize the two approaches. Which addresses deep
resolution of cycles and deferred error handling.
This also fixes a critical security vulnerability.
ValidateNoSetStateInEffects already supports transitive setter
functions. This PR marks any synchonous state setter useEffectEvent
function so we can validate that uEE isn't being used only as
misdirection to avoid the validation within an effect body.
The error points to the call of the effect event.
Example:
```js
export default function MyApp() {
const [count, setCount] = useState(0)
const effectEvent = useEffectEvent(() => {
setCount(10)
})
useEffect(() => {
effectEvent()
}, [])
return <div>{count}</div>;
```
```
Found 1 error:
Error: Calling setState synchronously within an effect can trigger cascading renders
Effects are intended to synchronize state between React and external systems such as manually updating the DOM, state management libraries, or other platform APIs. In general, the body of an effect should do one or both of the following:
* Update external systems with the latest state from React.
* Subscribe for updates from some external system, calling setState in a callback function when external state changes.
Calling setState synchronously within an effect body causes cascading renders that can hurt performance, and is not recommended. (https://react.dev/learn/you-might-not-need-an-effect).
5 | })
6 | useEffect(() => {
> 7 | effectEvent()
| ^^^^^^^^^^^ Avoid calling setState() directly within an effect
8 | }, [])
9 | return <div>{count}</div>;
10 | }
```
Fixes some issues i ran into w my recent snap changes:
* Correctly match against patterns that contain subdirectories, eg
`fbt/fbt-call`
* When checking if the input pattern has an extension, only prune known
supported extensions. Our convention of `error.<name>` for fixtures that
error makes the rest of the test name look like an extension to
`path.extname()`.
Tested with lots of different patterns including `error.` examples at
the top level and in nested directories, etc.
