Skip to content

Commit

Permalink
[compiler] Fix: ref.current now correctly reactive
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);
```
  • Loading branch information
mofeiZ committed Nov 12, 2024
1 parent 3770c11 commit 3669387
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 3669387

Please sign in to comment.