Draft
Conversation
Prompt: Looking at @compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/stable-handler/stable-handler-prop-basic.expect.md -- I see two potential problems. It looks to me like the onClick that is being passed to <button> will be re-created whenever `value` changes. It also looks like `t1` is only capturing the value of `onSubmit` when `value` changes. What we want instead is for `onClick` to never update, and to always capture the latest values of `value` and `onSubmit`.
Adds enableInferNonReactiveHandlers: a new pass in TypeInference that scans each FunctionExpression's uses within a component and, if every use is safe (JSX attribute or effect-hook argument), marks the function's identifiers as NonReactive. MarkNonReactiveScopes then drives the existing two-slot stable-identity codegen. Removes the need to manually annotate handlers with NonReactive<T> or wrap them in nonReactive() in the common case. - Bails out on direct calls, non-effect hook args (useMemo etc), object or array escape, return-from-component, and context flow. - Adds Reactive<T> as an opt-out annotation for cases where the inference would mark a handler but the user wants reactive identity. - Extends MarkNonReactiveScopes to recognize FunctionExpressions whose own lvalue is NonReactive-typed, so inline arrows in JSX attributes are also stabilized without an intermediate const assignment. - Drops the now-unused prop-level NonReactive<T> plumbing (propsTypeAnnotations on HIRFunction, extractPropsTypeAnnotations, InferTypes Destructure handling). Local NonReactive<T> annotations and the nonReactive() wrapper remain as explicit overrides. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Resolves conflicts with upstream's feature-flag cleanup (facebook#35825): - Keep local enableUseTypeAnnotations, enableNonReactiveAnnotation, and new enableInferNonReactiveHandlers flags in Environment.ts — our NonReactive work still needs them. - Accept upstream deletion of InferEffectDependencies.ts and the BuiltInAutodepsId / BuiltInEventHandlerId shapes (the autodeps/Fire feature was abandoned upstream). - Keep BuiltInNonReactiveId registration in ObjectShape.ts and add a matching BuiltInReactiveId registration for the Reactive<T> opt-out. - Drop dead isEventHandlerType helper from ValidateNoRefAccessInRender. - Re-add lowerType import in InferTypes.ts since our StoreLocal branch needs it. yarn snap: 1731 / 1731 passing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…facebook#36322) ## Summary Follow-up to facebook#36148 (which added credentialless as a recognized boolean attribute for iframes). Adds credentialless to possibleStandardNames so React's dev warning can suggest the correct casing when users write it as Credentialless (or another incorrect case). Includes an SSR test asserting the "Did you mean credentialless?" warning fires. ## Test plan - yarn test ReactDOMComponent passes, including the new should warn about incorrect casing on the credentialless property (ssr) case
…re options per DOM spec (facebook#36047) ## Summary `FragmentInstance.addEventListener` and `removeEventListener` fail to cross-match listeners when the `capture` option is passed as a **boolean** in one call and an **options object** in the other. This violates the [DOM Living Standard](https://dom.spec.whatwg.org/#dom-eventtarget-removeeventlistener), which states that `addEventListener(type, fn, true)` and `addEventListener(type, fn, {capture: true})` are identical. ### Root Cause In `ReactFiberConfigDOM.js`, the `normalizeListenerOptions` function generates a listener key string for deduplication. The boolean branch generates a **different format** than the object branch: ```js // Boolean branch (old) — produces "c=1" return `c=${opts ? '1' : '0'}`; // Object branch — produces "c=1&o=0&p=0" return `c=${opts.capture ? '1' : '0'}&o=${opts.once ? '1' : '0'}&p=${opts.passive ? '1' : '0'}`; ``` Because the keys differ, `indexOfEventListener` cannot match them — so `removeEventListener('click', fn, {capture: true})` silently fails to remove a listener registered with `addEventListener('click', fn, true)`, and vice versa. This causes a **memory leak and event listener accumulation** on all Fragment child DOM nodes. ### Fix Normalize the boolean branch to produce the same full key format: ```js // Boolean branch (fixed) — now produces "c=1&o=0&p=0" (matches object branch) return `c=${opts ? '1' : '0'}&o=0&p=0`; ``` This makes both forms produce an identical key, matching the DOM spec behavior. ### When Was This Introduced This bug has been present since `FragmentInstance` event listener tracking was first added. It became reachable in production as of [facebook#36026](facebook#36026) which enabled `enableFragmentRefs` + `enableFragmentRefsInstanceHandles` across all builds (merged 3 days ago). ### Tests Added two new regression tests to `ReactDOMFragmentRefs-test.js`: 1. `removes a capture listener registered with boolean when removed with options object` 2. `removes a capture listener registered with options object when removed with boolean` Both tests were failing before this fix and pass after. ## How did you test this change? Added two new automated tests covering both cross-form removal directions. Existing tests continue to pass. ## Changelog ### React DOM - **Fixed** `FragmentInstance.removeEventListener()` not removing capture-phase listeners when the `capture` option form (boolean vs options object) differs between `add` and `remove` calls.
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
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.
Summary
Automatic stable-identity codegen for handler-style functions in components, gated behind new feature flags. The goal is to reduce the amount of manual memoization (
useCallback, custom annotations) users have to write for the common case of "inline function that's only passed as a JSX prop."There are two layers:
1.
NonReactive<T>annotation + two-slot codegen (already in earlier commits on this branch)Users can type a local handler as
NonReactive<T>(or wrap it withnonReactive(...)). The compiler then emits a two-slot cache for that function:The wrapper has stable identity for downstream consumers but always delegates to the current closure, so captured values stay fresh.
This powers: excluding the handler from reactive deps (
InferReactivePlaces,ValidateExhaustiveDependencies,InferEffectDependencies), and enabling ref-access through it.2. Usage-based inference (new in this PR's latest commit)
Gated behind
enableInferNonReactiveHandlers. A new passinferNonReactiveHandlersruns afterinferTypesand, for eachFunctionExpressionin the component, checks whether every transitive use flows only into safe sinks:useEffect/useLayoutEffect/useInsertionEffect/useEffectEventargument, aliasing transfers (LoadLocal / StoreLocal / phi).useMemo,useCallback,useState, custom hooks), stored in an object or array (escape), returned from the component, flowed through a context variable, or an explicitReactive<T>opt-out annotation.If every use is safe, the function's identifier types are mutated to
BuiltInNonReactiveand the existingmarkNonReactiveScopespass drives the two-slot codegen unchanged. Also handles inline arrows in JSX (no intermediateconstrequired) via a small extension toMarkNonReactiveScopes.Escape hatches
Reactive<T>annotation on a local to opt out of the inference (preferred — more grokkable at the call site)useCallbackstill worksNonReactive<T>annotation andnonReactive()wrapper remain as forceful opt-ins for cases the inference bails out onThe prop-level
NonReactive<T>annotation (previously on component parameters) was removed in this PR — the use cases (cross-module stability guarantees, suppressing Child's exhaustive-deps warnings, ref access through a passed-in prop) were deemed not worth the complexity right now.Feature flags
enableNonReactiveAnnotation— existing gate for the annotation + two-slot codegen.enableUseTypeAnnotations— prerequisite for readingNonReactive<T>/Reactive<T>off variable declarations.enableInferNonReactiveHandlers— new flag for the usage-based inference. RequiresenableNonReactiveAnnotation.All default to
false— no behavior change for existing users.How did you test this change?
yarn snap).yarn tsc --noEmitclean.compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/non-reactive/:infer-non-reactive-inline-handler— baseline:const h = () => …; <button onClick={h}/>→ two-slot codegeninfer-non-reactive-inline-jsx— inline arrow directly in JSX attribute → two-slotinfer-non-reactive-aliased— handler relayed through a second localinfer-non-reactive-reactive-optout—Reactive<T>keeps the value-cached codegeninfer-non-reactive-bailout-local-call— calling the handler in render bails outinfer-non-reactive-bailout-usememo— passing touseMemobails outinfer-non-reactive-bailout-object-escape— storing in an object bails outnon-reactive-prop-propagation— cross-component: Parent's handler gets stabilized, Child caches its JSX on a stable identity so it never invalidates