-
Notifications
You must be signed in to change notification settings - Fork 47.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[compiler] Fix: ref.current now correctly reactive #31521
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -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: [], | ||
}; | ||
Comment on lines
+549
to
+552
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now we explicitly downgrade |
||
} | ||
if (this.#checkValidDependency(maybeDependency)) { | ||
this.#dependencies.value!.push(maybeDependency); | ||
} | ||
|
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> | ||
Comment on lines
+101
to
+102
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that here, the re-render receives a different |
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}], | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a little weird. Ideally we should take the
ref
type producing theref.current
read as a dependency, but this seems like an edge case (we don't allowref.current
reads in render anyways).Happy to turn this into an invariant instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah interesting. seems worth trying to add an invariant here in a follow-up and see if there's any place it gets hit