-
Notifications
You must be signed in to change notification settings - Fork 1.4k
feat: reactive var props #8156
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
feat: reactive var props #8156
Conversation
🦋 Changeset detectedLatest commit: 86e3a98 The changes in this PR will be included in the next version bump. 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 |
577abec to
56454b0
Compare
commit: |
built with Refined Cloudflare Pages Action⚡ Cloudflare Pages Deployment
|
| let propsAreDifferent = false; | ||
| if (vNodeProps) { | ||
| const effects = vNodeProps[_PROPS_HANDLER].$effects$; | ||
| const constPropsDifferent = handleChangedProps( |
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.
let's only do this if the key is different
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.
but to know if key is changed we need to run it anyway
| // to this signal. | ||
| ensureContainsBackRef(effectSubscription, target); | ||
| addQrlToSerializationCtx(effectSubscription, store.$container$); | ||
| addQrlToSerializationCtx(effectSubscription, '$container$' in store ? store.$container$ : null); |
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.
add a comment here to say it's to avoid subscribing?
| const owner = value[_OWNER]; | ||
| output(TypeIds.PropsProxy, [_serializationWeakRef(owner), owner.varProps, owner.constProps]); | ||
| output(TypeIds.PropsProxy, [ | ||
| _serializationWeakRef(owner), |
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.
I think we should take this out, and instead add a _serializationWeakRef(propsProxy) to jsxNode, and do propsProxy.owner = jsxNode in inflate of jsxNode
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.
you cant have varprops and const props without owner, so how we can inflate propsproxy then?
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.
hmm ok to fix this we have to move ownership of the props to propsproxy, as soon as it is created. But we shouldn't create it for every jsxnode so we must support both paths.
I think that can go in another PR.
a05bcd7 to
5746b46
Compare
wmertens
left a comment
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.

Make props proxy more reactive by adding effects for var props like a store.