-
Notifications
You must be signed in to change notification settings - Fork 1.4k
fix: serialize correctly null or undefined value for signals #8160
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
Conversation
🦋 Changeset detectedLatest commit: 1fc309c 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 |
9b8904d to
6d70fd3
Compare
commit: |
built with Refined Cloudflare Pages Action⚡ Cloudflare Pages Deployment
|
6d70fd3 to
eb69e4b
Compare
| } else { | ||
| output(TypeIds.Signal, [value.$untrackedValue$, ...(value.$effects$ || [])]); | ||
| const v = value.$untrackedValue$; | ||
| const keepUndefined = v === undefined; |
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 don't think this is needed, [][0] is undefined
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.
it is needed, but you are right, its bugged. It should keep only value if effects are undefined. I pushed some changes
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 don't get it, if you don't keep undefined, it will be an empty array and it won't create the effects set?
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.
its for value, not effects
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.
oh right, missing value means invalid, not undefined
eb69e4b to
1fc309c
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.

Correctly serialize null or undefined value.
Remove only undefined values during serialization.
Make default undefined value for effects and back refs instead of null