docs(ngrx/signals): mention re-use of withComputed/withMethods
#4780
Conversation
… for re-use Issue: ngrx#4669
✅ Deploy Preview for ngrx-io ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
❌ Deploy Preview for ngrx-site-v19 failed.
|
|
@timdeschryver @rainerhahnekamp since you two coached me in the issue I tweaked the example code comments a little. I added the block towards the end of the "Core Concepts > Defining Store Methods" as this pertains to both |
| withComputed(({ books, filter }) => ({ | ||
| booksCount: computed(() => books().length), | ||
| sortDirection: computed(() => filter.order() === 'asc' ? 1 : -1), | ||
| })), | ||
| // 👇 (A) Also access previously defined computed properties (or functions). | ||
| withComputed(({ books, sortDirection }) => { |
There was a problem hiding this comment.
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:
withComputed({ books, filter }) => {
const booksCount = computed(() => books().length);
const sortDirection = computed(() => /* ... */);
// ...
return { /* export public computed signals */ };
}),Additionally, it provides the ability not to export private computed signals if necessary.
There was a problem hiding this comment.
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 {} }.
(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'm not a fan of using multiple withComputed calls. The native JS way should be preferred and recommended in my opinion:
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 the const method works good but I prefer multiple withComputed". So in my opinion, I feel like this const helper restructuring is worth a tip somewhere and that people who prefer multiple withComputed are just going to keep doing that lol.
There was a problem hiding this comment.
What about having this PR as still adding this tip with respect to the native const approach, but dropping the repeated withComputed/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.
Issue: #4669
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue: docs(@ngrx/signals): add a mention of restructuring withComputed or withMethods for re-use in those features
Closes #4669
What is the new behavior?
Documentation mentions how to re-use a computed or function across or within a
withComputedorwithMethodsDoes this PR introduce a breaking change?