Need this to run against target for forks to get the notification.
This job does not checkout the code in the PR, so it's safe to run from
the target.
Also fixes failing checks on PRs:
<img width="870" alt="Screenshot 2025-03-24 at 3 28 30 PM"
src="https://github.com/user-attachments/assets/add78287-6449-4e48-9376-f3b360d2607c"
/>
(Found when compiling Meta React code)
Let variable declarations and reassignments are currently rewritten to
`StoreLocal <varName>` instructions, which each translates to a new
`const varName` declaration in codegen.
```js
// Example input
function useHook() {
const getX = () => x;
let x = CONSTANT1;
if (cond) {
x += CONSTANT2;
}
return <Stringify getX={getX} />
}
// Compiled output, prior to this PR
import { c as _c } from "react/compiler-runtime";
function useHook() {
const $ = _c(1);
let t0;
if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
const getX = () => x;
let x = CONSTANT1;
if (cond) {
let x = x + CONSTANT2;
x;
}
t0 = <Stringify getX={getX} />;
$[0] = t0;
} else {
t0 = $[0];
}
return t0;
}
```
This also manifests as a babel internal error when replacing the
original function declaration with the compiler output. The below
compilation output fails with `Duplicate declaration "x" (This is an
error on an internal node. Probably an internal error.)`.
```js
// example input
let x = CONSTANT1;
if (cond) {
x += CONSTANT2;
x = CONSTANT3;
}
// current output
let x = CONSTANT1;
if (playheadDragState) {
let x = x + CONSTANT2
x;
let x = CONSTANT3;
}
```
We currently have the ability to have a separate animation for a
ViewTransition that relayouts but doesn't actually have any internal
mutations. This can be useful if you want to separate just a move from
for example flashing an update.
However, we're concerned that this might be more confusion than its
worth because subtle differences in mutations can cause it to trigger
the other case. The existence of the property name might also make you
start looking for it to solve something that it's not meant for.
We already fallback to using the "update" property if it exists but
layout doesn't. So if we ever decide to add this back it would backwards
compatible. We've also shown in implementation that it can work.
This implements `getRootNode(options)` on fragment instances as the
equivalent of calling `getRootNode` on the fragment's parent host node.
The parent host instance will also be used to proxy dispatchEvent in an
upcoming PR.
Avoid failing builds when imported function specifiers conflict by using
babel's `generateUid`. Failing a build is very disruptive, as it usually
presents to developers similar to a javascript parse error.
```js
import {logRender as _logRender} from 'instrument-runtime';
const logRender = () => { /* local conflicting implementation */ }
function Component_optimized() {
_logRender(); // inserted by compiler
}
```
Currently, we fail builds (even in `panicThreshold:none` cases) when
import specifiers are detected to conflict with existing local
variables. The reason we destructively throw (instead of bailing out) is
because (1) we first generate identifier references to the conflicting
name in compiled functions, (2) replaced original functions with
compiled functions, and then (3) finally check for conflicts.
When we finally check for conflicts, it's too late to bail out.
```js
// import {logRender} from 'instrument-runtime';
const logRender = () => { /* local conflicting implementation */ }
function Component_optimized() {
logRender(); // inserted by compiler
}
```
Adds Effect.ConditionallyMutateIterator, which has the following
effects:
- capture for known array, map, and sets
- mutate for all other values
An alternative to this approach could be to add polymorphic shape
definitions
* Adds `isConstructor: boolean` to `FunctionType`. With this PR, each
typed function can either be a constructor (currently only known
globals) or non constructor. Alternatively, we prefer to encode
polymorphic types / effects (and match the closest subtype)
* Add Map and Set globals + built-ins
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/32697).
* #32698
* __->__ #32697
`pull_request_target` gives access to repository secrets and permissions
for use from forks, for example to add a comment.
> Due to the dangers inherent to automatic processing of PRs, GitHub’s
standard pull_request workflow trigger by default prevents write
permissions and secrets access to the target repository. However, in
some scenarios such access is needed to properly process the PR. To this
end the pull_request_target workflow trigger was introduced.
> The reason to introduce the pull_request_target trigger was to enable
workflows to label PRs (e.g. needs review) or to comment on the PR.
(via
https://securitylab.github.com/resources/github-actions-preventing-pwn-requests/)
In this case there is no reason for us to allow this, so let's just use
the normal `pull_request` trigger which is less permissive.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/32708).
* __->__ #32708
* #32709
Followup from https://github.com/facebook/react/pull/32499
Manual mode is unused and has some bugs such as revealing hidden
boundaries when manually toggling. We also want to change how manual
mode works, and do some refactors to Activity to make it easier to
support. For now we'll remove it, then add it back after the other
changes we have planned.
This works around this Safari bug.
https://bugs.webkit.org/show_bug.cgi?id=290146
This unfortunate because it may cause additional layouts if there's more
updates to the tree coming by manual mutation before it gets painted
naturally. However, we might end up wanting to read layout early anyway.
This affects the fixture because we clone the `<link>` from the `<head>`
which is itself another bug. However, it should be possible to have
`<link>` tags inserted into the new tree so this is still relevant.
Partially reverts #32686.
PR caches inherit from caches generated in `main`. If it cannot find
that cache, it will create one scoped to just that PR (and PRs that
inherit from it).
There is an edge case where cache eviction can happen in the middle of a
test run. If cache eviction removes a `main` cache, child jobs that
depend on it will start failing because of the `fail-on-cache-miss`
setting.
This PR reverts the default behavior. If this happens, the workflow will
still continue in slow mode where it will `yarn install` child jobs
instead of reusing from cache. This is slower but will at least allow
workflows to continue.
Additionally I added restore keys so that we can fallback to other
caches if present so `yarn install` doesn't need to start over from
scratch.
Updates ~all of our validations to return a Result, and then updates callers to either unwrap() if they should bailout or else just log.
ghstack-source-id: 418b5f5aa2b7dd49ca76b3f98a48a35150691d7e
Pull Request resolved: https://github.com/facebook/react/pull/32688
React uses function identity to determine whether a given JSX expression represents the same type of component and should reconcile (keep state, update props) or replace (teardown state, create a new instance). This PR adds off-by-default validation to check that developers are not dynamically creating components during render.
The check is local and intentionally conservative. We specifically look for the results of call expressions, new expressions, or function expressions that are then used directly (or aliased) as a JSX tag. This allows common sketchy but fine-in-practice cases like passing a reference to a component from a parent as props, but catches very obvious mistakes such as:
```js
function Example() {
const Component = createComponent();
return <Component />;
}
```
We could expand this to catch more cases, but this seems like a reasonable starting point. Note that I tried enabling the validation by default and the only fixtures that error are the new ones added here. I'll also test this internally. What i'm imagining is that we enable this in the linter but not the compiler.
ghstack-source-id: e7408c0a55478b40d65489703d209e8fa7205e45
Pull Request resolved: https://github.com/facebook/react/pull/32683
Since we use a centralized cache we should fail subsequent steps if the
child jobs are unable to restore the cache from the first 2 jobs.
Also fix some incorrect hashes used for the fixture tests.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/32686).
* __->__ #32686
* #32685
There was a bug previously in our commit artifacts step where the
emitted REVISION hash would reference the commit on the builds branch
rather than from `main`.
Given that our internal manual sync script also does this, let's align
them both to always reference the commit from `main` instead.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/32680).
* __->__ #32680
* #32679
* #32678
Defaults to warn, but since some steps require these artifacts to be
uploaded we specify an error if its not found. Some other steps like
playwright test-results are only uploaded on failure so it's okay to
ignore.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/32679).
* #32680
* __->__ #32679
* #32678
> Caches have branch scope restriction in place. This means that if
caches for a specific branch are using a lot of storage quota, it may
result into more frequently used caches from default branch getting
thrashed. For example, if there are many pull requests happening on a
repo and are creating caches, these cannot be used in default branch
scope but will still occupy a lot of space till they get cleaned up by
eviction policy. But sometime we want to clean them up on a faster
cadence so as to ensure default branch is not thrashing.
https://github.com/actions/cache/blob/main/tips-and-workarounds.md#force-deletion-of-caches-overriding-default-cache-eviction-policy
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/32675).
* __->__ #32675
* #32674
To avoid race conditions where multiple jobs try to write to the same
cache, we now centralize saving the cache and then reusing it in every
subsequent job.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/32672).
* #32675
* #32674
* __->__ #32672
## Summary
In the recommended configuration for `eslint-plugin-react-compiler`,
i.e. `reactCompiler.configs.recommended`, the rule is typed as `string`
rather than `eslint.Linter.RuleEntry` or anything assignable thereto,
which results in the following type error if you type check your eslint
configuration:
```
Property ''react-compiler/react-compiler'' is incompatible with index signature.
Type 'string' is not assignable to type 'RuleEntry | undefined'.
```
Simply adding a const assertion fixes the error.
## How did you test this change?
I emitted declarations for the module and confirmed that the rule is now
typed as the string literal `'error'`
I'm seeing a lot of instances of
> Failed to save: Unable to reserve cache with key
runtime-and-compiler-node_modules-v5-X64-Linux-e454609794aae66da9909c77dd6efa073eceff7f44d6527611f8465e102578b4,
another job may be creating this cache.
which is adding ~20 seconds to every step. Let's try to bust the cache
following this
[comment](https://github.com/actions/cache/issues/485#issuecomment-744145040)
and see if that helps.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/32671).
* #32672
* __->__ #32671
Using the github variable for the commit message replaces the variable
inline. If the commit message contains quotes or other characters that
need to be escaped, this breaks the workflow.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/32668).
* #32669
* __->__ #32668
Follow up to #32656.
Remove touchAction from SwipeRecognizer. I was under the wrong
impression that this was only the touch-action applied to this
particular element, but that parents would still win but in fact this
blocks the parent from scrolling in the other direction. By specifying a
fixed direction it also blocked rage-swiping in the other direction
early on.
Disable pointer-events on view-transition so that the scroll can be hit.
This means that touches hit below the items animating above. This allows
swiping to happen again before momentum scroll has finished. Previously
they were ignored. This only works as long as the SwipeRecognizer is
itself not animating. This means you can now rage-swipe in both
directions quickly.
Alternative to facebook/react#31584 which sets
enableTreatFunctionDepsAsConditional:true` by default.
This PR changes dependency hoisting to be more conservative while trying
to preserve an optimal "happy path". We assume that a function "is
likely called" if we observe the following in the react function body.
- a direct callsite
- passed directly as a jsx attribute or child
- passed directly to a hook
- a direct return
A function is also "likely called" if it is directly called, passed to
jsx / hooks, or returned from another function that "is likely called".
Note that this approach marks the function definition site with its
hoistable properties (not its use site). I tried implementing use-site
hoisting semantics, but it felt both unpredictable (i.e. as a developer,
I can't trust that callbacks are well memoized) and not helpful (type +
null checks of a value are usually colocated with their use site)
In this fixture (copied here for easy reference), it should be safe to
use `a.value` and `b.value` as dependencies, even though these functions
are conditionally called.
```js
// inner-function/nullable-objects/assume-invoked/conditional-call-chain.tsx
function Component({a, b}) {
const logA = () => {
console.log(a.value);
};
const logB = () => {
console.log(b.value);
};
const hasLogged = useRef(false);
const log = () => {
if (!hasLogged.current) {
logA();
logB();
hasLogged.current = true;
}
};
return <Stringify log={log} shouldInvokeFns={true} />;
}
```
On the other hand, this means that we produce invalid output for code
like manually implementing `Array.map`
```js
// inner-function/nullable-objects/bug-invalid-array-map-manual.js
function useFoo({arr1, arr2}) {
const cb = e => arr2[0].value + e.value;
const y = [];
for (let i = 0; i < arr1.length; i++) {
y.push(cb(arr1[i]));
}
return y;
}
```
