fix: correctly inspect derived values #9731
Conversation
🦋 Changeset detectedLatest commit: 6e74c75 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| if (DEV) { | ||
| return { | ||
| // block | ||
| b: block, | ||
| // consumers | ||
| c: null, | ||
| // destroy | ||
| d: null, | ||
| // equals | ||
| e: null, | ||
| // flags | ||
| f: flags, | ||
| // init | ||
| i: null, | ||
| // references | ||
| r: null, | ||
| // value | ||
| v: value, | ||
| // context: We can remove this if we get rid of beforeUpdate/afterUpdate | ||
| x: null, | ||
| // destroy | ||
| y: null, | ||
| // this is for DEV only | ||
| inspect: new Set() | ||
| }; | ||
| } | ||
|
|
There was a problem hiding this comment.
Nit: might be nicer to abstract this to a base_signal const and just add the inspect property, rather than repeating the entire object twice.
i.e.
function create_computation_signal(flags, value, block) {
const base_signal = {
// block
b: block,
// consumers
c: null,
// destroy
d: null,
// equals
e: null,
// flags
f: flags,
// init
i: null,
// references
r: null,
// value
v: value,
// context: We can remove this if we get rid of beforeUpdate/afterUpdate
x: null,
// destroy
y: null
};
if (DEV) {
return {
...base_signal,
// this is for DEV only
inspect: new Set()
};
}
return base_signal;
}Same goes for the create_source_signal implementation.
There was a problem hiding this comment.
My initial thought was to do this...
if (DEV) {
signal.inspect = ...
}...but the reason we don't do that is that you're changing the object shape when you do that, which IIUC (though @trueadm is the authority on such things) makes subsequent reads slower.
The spread approach should avoid that problem, though it does mean that you have to create the signal twice. It's not the worst thing in the world since it's only in dev, but ideally dev would resemble prod as closely as is practical. I'll defer to @trueadm on this question.
There was a problem hiding this comment.
Yeah, I think this acceptable.
| if (DEV) { | ||
| return { | ||
| // block | ||
| b: block, | ||
| // consumers | ||
| c: null, | ||
| // destroy | ||
| d: null, | ||
| // equals | ||
| e: null, | ||
| // flags | ||
| f: flags, | ||
| // init | ||
| i: null, | ||
| // references | ||
| r: null, | ||
| // value | ||
| v: value, | ||
| // context: We can remove this if we get rid of beforeUpdate/afterUpdate | ||
| x: null, | ||
| // destroy | ||
| y: null, | ||
| // this is for DEV only | ||
| inspect: new Set() | ||
| }; | ||
| } | ||
|
|
There was a problem hiding this comment.
Yeah, I think this acceptable.
Closes #9728
Before submitting the PR, please make sure you do the following
feat:,fix:,chore:, ordocs:.Tests and linting
pnpm testand lint the project withpnpm lint