Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
solid-primitives destructure: Added new config option "normalize" #525
base: main
Are you sure you want to change the base?
solid-primitives destructure: Added new config option "normalize" #525
Changes from 11 commits
d43a7ab
db9a9da
ff9b2b9
5c37513
484827d
da64266
c693954
150b1df
8cf7040
15438fc
6cd141e
9717358
98f999e
2159512
4d0e264
f4eb72f
b56f253
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
whats the reason for memoizing the source? in most cases the primitive is used with props object or a signal, where the source memoization is useless.
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.
There is one thing I'm realizing about this.
Even if we want to avoid
cb()(1, 2)
syntax of normal destructure, maybe it should still support "reactive callbacks", the sameprops.callback(1, 2)
would if you passedcallback={someSignal()}
in JSX.Although Solid doesn't allow for reactive callbacks in
on___
props, so it may be fine to restrict that as well.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 can spit it to two different tests as you are testing two things.
"@thetarnav's stuff"
and"@madaxen86's stuff"
. Could be more descriptive btw 😄I'm aware what my test is doing (testing combining with the memo option enabled and disabled), so I'm guessing the second test is for a function source and deep: true?
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.
if you replace it with
c: () => (a: number, b: number) => a * b
the types break, but tests still passThere 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 have adjusted the types.
It became a bit messy but now the types are inferred correctly.