-
-
Notifications
You must be signed in to change notification settings - Fork 2k
docs(ngrx/signals): mention re-use of withComputed/withMethods
#4780
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
Open
michael-small
wants to merge
1
commit into
ngrx:main
Choose a base branch
from
michael-small:docs-reference-other-computed-or-function
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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.
When I was writing this page, I assumed that readers are familiar with core JavaScript concepts, so they're aware that
() => ({})
is a sugar syntax for() => { return {} }
.I'm not a fan of using multiple
withComputed
calls. The native JS way should be preferred and recommended in my opinion:Additionally, it provides the ability not to export private computed signals if necessary.
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.
(sorry if this is overselling but I have thought on this a lot) In retrospect that made sense when I learned this after seeking it out, but I have seen people with this same problem frequently across different platforms. It has even been news to some GDEs who make signal store content. I can't speak for why the various other people didn't recognize this sugar syntax, but for me I just think going from a class based store to a function based one has a lot of little moments like this. And this point for people who wonder about this seems to solve a large hurdle with a small tip.
I agree. That said, in the places this has come up, it feels like 2/3 of people are like "great, no need for another
withComputed
" and the other 1/3 are like "that's cool that theconst
method works good but I prefer multiplewithComputed
". So in my opinion, I feel like thisconst
helper restructuring is worth a tip somewhere and that people who prefer multiplewithComputed
are just going to keep doing that lol.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.
What about having this PR as still adding this tip with respect to the native
const
approach, but dropping the repeatedwithComputed/withMethods
reference?@timdeschryver / @rainerhahnekamp you both suggested having them both, so I'm curious if my proposed compromise which tentatively drop the repeated
withComputed
sounds legit.