Skip to content

Commit

Permalink
[compiler] Fix: ref.current now correctly reactive (#31521)
Browse files Browse the repository at this point in the history
We were previously filtering out `ref.current` dependencies in
propagateScopeDependencies:checkValidDependency`. This is incorrect.

Instead, we now always take a dependency on ref values (the outer box)
as they may be reactive. Pruning is done in
pruneNonReactiveDependencies.

This PR includes a small patch to `collectReactiveIdentifier`. Prior to
this, we conservatively assumed that pruned scopes always produced
reactive declarations. This assumption fixed a bug with non-reactivity,
but some of these declarations are `useRef` calls. Now we have special
handling for this case
```js
// This often produces a pruned scope
React.useRef(1);
```
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/31521).
* #31202
* #31203
* #31201
* #31200
* __->__ #31521
  • Loading branch information
mofeiZ authored Nov 15, 2024
1 parent 3720870 commit 4972718
Show file tree
Hide file tree
Showing 11 changed files with 272 additions and 147 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -440,14 +440,6 @@ class Context {

// Checks if identifier is a valid dependency in the current scope
#checkValidDependency(maybeDependency: ReactiveScopeDependency): boolean {
// ref.current access is not a valid dep
if (
isUseRefType(maybeDependency.identifier) &&
maybeDependency.path.at(0)?.property === 'current'
) {
return false;
}

// ref value is not a valid dep
if (isRefValueType(maybeDependency.identifier)) {
return false;
Expand Down Expand Up @@ -549,6 +541,16 @@ class Context {
});
}

// ref.current access is not a valid dep
if (
isUseRefType(maybeDependency.identifier) &&
maybeDependency.path.at(0)?.property === 'current'
) {
maybeDependency = {
identifier: maybeDependency.identifier,
path: [],
};
}
if (this.#checkValidDependency(maybeDependency)) {
this.#dependencies.value!.push(maybeDependency);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ import {
PrunedReactiveScopeBlock,
ReactiveFunction,
isPrimitiveType,
isUseRefType,
Identifier,
} from '../HIR/HIR';
import {ReactiveFunctionVisitor, visitReactiveFunction} from './visitors';

Expand Down Expand Up @@ -50,13 +52,21 @@ class Visitor extends ReactiveFunctionVisitor<Set<IdentifierId>> {
this.traversePrunedScope(scopeBlock, state);

for (const [id, decl] of scopeBlock.scope.declarations) {
if (!isPrimitiveType(decl.identifier)) {
if (
!isPrimitiveType(decl.identifier) &&
!isStableRefType(decl.identifier, state)
) {
state.add(id);
}
}
}
}

function isStableRefType(
identifier: Identifier,
reactiveIdentifiers: Set<IdentifierId>,
): boolean {
return isUseRefType(identifier) && !reactiveIdentifiers.has(identifier.id);
}
/*
* Computes a set of identifiers which are reactive, using the analysis previously performed
* in `InferReactivePlaces`.
Expand Down

This file was deleted.

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@

## Input

```javascript
import {useRef, forwardRef} from 'react';
import {Stringify} from 'shared-runtime';

/**
* Fixture showing that Ref types may be reactive.
* We should always take a dependency on ref values (the outer box) as
* they may be reactive. Pruning should be done in
* `pruneNonReactiveDependencies`
*/

function Parent({cond}) {
const ref1 = useRef(1);
const ref2 = useRef(2);
const ref = cond ? ref1 : ref2;
return <Child ref={ref} />;
}

function ChildImpl(_props, ref) {
const cb = () => ref.current;
return <Stringify cb={cb} shouldInvokeFns={true} />;
}

const Child = forwardRef(ChildImpl);

export const FIXTURE_ENTRYPOINT = {
fn: Parent,
params: [{cond: true}],
sequentialRenders: [{cond: true}, {cond: false}],
};

```

## Code

```javascript
import { c as _c } from "react/compiler-runtime";
import { useRef, forwardRef } from "react";
import { Stringify } from "shared-runtime";

/**
* Fixture showing that Ref types may be reactive.
* We should always take a dependency on ref values (the outer box) as
* they may be reactive. Pruning should be done in
* `pruneNonReactiveDependencies`
*/

function Parent(t0) {
const $ = _c(2);
const { cond } = t0;
const ref1 = useRef(1);
const ref2 = useRef(2);
const ref = cond ? ref1 : ref2;
let t1;
if ($[0] !== ref) {
t1 = <Child ref={ref} />;
$[0] = ref;
$[1] = t1;
} else {
t1 = $[1];
}
return t1;
}

function ChildImpl(_props, ref) {
const $ = _c(4);
let t0;
if ($[0] !== ref) {
t0 = () => ref.current;
$[0] = ref;
$[1] = t0;
} else {
t0 = $[1];
}
const cb = t0;
let t1;
if ($[2] !== cb) {
t1 = <Stringify cb={cb} shouldInvokeFns={true} />;
$[2] = cb;
$[3] = t1;
} else {
t1 = $[3];
}
return t1;
}

const Child = forwardRef(ChildImpl);

export const FIXTURE_ENTRYPOINT = {
fn: Parent,
params: [{ cond: true }],
sequentialRenders: [{ cond: true }, { cond: false }],
};

```
### Eval output
(kind: ok) <div>{"cb":{"kind":"Function","result":1},"shouldInvokeFns":true}</div>
<div>{"cb":{"kind":"Function","result":2},"shouldInvokeFns":true}</div>
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
import {useRef, forwardRef} from 'react';
import {Stringify} from 'shared-runtime';

/**
* Fixture showing that Ref types may be reactive.
* We should always take a dependency on ref values (the outer box) as
* they may be reactive. Pruning should be done in
* `pruneNonReactiveDependencies`
*/

function Parent({cond}) {
const ref1 = useRef(1);
const ref2 = useRef(2);
const ref = cond ? ref1 : ref2;
return <Child ref={ref} />;
}

function ChildImpl(_props, ref) {
const cb = () => ref.current;
return <Stringify cb={cb} shouldInvokeFns={true} />;
}

const Child = forwardRef(ChildImpl);

export const FIXTURE_ENTRYPOINT = {
fn: Parent,
params: [{cond: true}],
sequentialRenders: [{cond: true}, {cond: false}],
};
Loading

0 comments on commit 4972718

Please sign in to comment.