Skip to content

Commit

Permalink
[compiler] Don't include current field accesses in auto-deps
Browse files Browse the repository at this point in the history
## Summary

Drops .current field accesses in inferred dep arrays
  • Loading branch information
jbrown215 committed Dec 2, 2024
1 parent 5b0ef21 commit 9bcf3d1
Show file tree
Hide file tree
Showing 3 changed files with 123 additions and 0 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,13 @@ function writeDependencyToInstructions(
*/
break;
}
if (path.property === 'current') {
/*
* Prune ref.current accesses. This may over-capture for non-ref values with
* a current property, but that's fine.
*/
break;
}
const nextValue = createTemporaryPlace(env, GeneratedSource);
nextValue.reactive = reactive;
instructions.push({
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@

## Input

```javascript
// @inferEffectDependencies
import {useEffect} from 'react';
import {print} from 'shared-runtime';

/**
* We never include a .current access in a dep array because it may be a ref access.
* This might over-capture objects that are not refs and happen to have fields named
* current, but that should be a rare case and the result would still be correct
* (assuming the effect is idempotent). In the worst case, you can always write a manual
* dep array.
*/
function RefsInEffects() {
const ref = useRefHelper();
const wrapped = useDeeperRefHelper();
useEffect(() => {
print(ref.current);
print(wrapped.foo.current);
});
}

function useRefHelper() {
return useRef(0);
}

function useDeeperRefHelper() {
return {foo: useRefHelper()};
}

```

## Code

```javascript
import { c as _c } from "react/compiler-runtime"; // @inferEffectDependencies
import { useEffect } from "react";
import { print } from "shared-runtime";

/**
* We never include a .current access in a dep array because it may be a ref access.
* This might over-capture objects that are not refs and happen to have fields named
* current, but that should be a rare case and the result would still be correct
* (assuming the effect is idempotent). In the worst case, you can always write a manual
* dep array.
*/
function RefsInEffects() {
const $ = _c(3);
const ref = useRefHelper();
const wrapped = useDeeperRefHelper();
let t0;
if ($[0] !== ref.current || $[1] !== wrapped.foo.current) {
t0 = () => {
print(ref.current);
print(wrapped.foo.current);
};
$[0] = ref.current;
$[1] = wrapped.foo.current;
$[2] = t0;
} else {
t0 = $[2];
}
useEffect(t0, [ref, wrapped.foo]);
}

function useRefHelper() {
return useRef(0);
}

function useDeeperRefHelper() {
const $ = _c(2);
const t0 = useRefHelper();
let t1;
if ($[0] !== t0) {
t1 = { foo: t0 };
$[0] = t0;
$[1] = t1;
} else {
t1 = $[1];
}
return t1;
}

```
### Eval output
(kind: exception) Fixture not implemented
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
// @inferEffectDependencies
import {useEffect} from 'react';
import {print} from 'shared-runtime';

/**
* We never include a .current access in a dep array because it may be a ref access.
* This might over-capture objects that are not refs and happen to have fields named
* current, but that should be a rare case and the result would still be correct
* (assuming the effect is idempotent). In the worst case, you can always write a manual
* dep array.
*/
function RefsInEffects() {
const ref = useRefHelper();
const wrapped = useDeeperRefHelper();
useEffect(() => {
print(ref.current);
print(wrapped.foo.current);
});
}

function useRefHelper() {
return useRef(0);
}

function useDeeperRefHelper() {
return {foo: useRefHelper()};
}

0 comments on commit 9bcf3d1

Please sign in to comment.