feat/resource/value as proxy #270
Conversation
- fix withEntityResource - add additional type tests - make `undefined value` as the default error handling
| errorHandling: ErrorHandling, | ||
| ): WritableSignal<T>; | ||
|
|
||
| function valueSignalForErrorHandling<T>( |
There was a problem hiding this comment.
@wolfmanfx I checked the possibility for replacing the function reference, but since a Signal is a function with properties and methods, the code became quite messy. So I stayed with the Proxy approach.
michael-small
left a comment
michael-small
left a comment
There was a problem hiding this comment.
Besides a call for named withEntityResource tests for error handling, most other comments are nitpicks of jsdocs or pre-existing TODO comments. But all the business logic looks solid.
| warningSpy.mockClear(); | ||
| }); | ||
|
|
||
| //TODO wait for https://github.com/ngrx/platform/pull/4932 and then add 'value' to the list |
There was a problem hiding this comment.
Old outstanding TODO
There was a problem hiding this comment.
I don't think it belongs in this PR since it would not be possible to backport it to v20. I would recommend doing a separate PR for that. If you want you can do it.
There was a problem hiding this comment.
No big deal, some other time then
| }, | ||
| ); | ||
|
|
||
| //TODO wait for https://github.com/ngrx/platform/pull/4932 |
There was a problem hiding this comment.
Old outstanding TODO
There was a problem hiding this comment.
Separate issue (see above)
| }); | ||
| }); | ||
|
|
||
| describe('Signature Tests', () => { |
There was a problem hiding this comment.
Needs named equivalents right?
Co-authored-by: Michael Small <[email protected]>
Co-authored-by: Michael Small <[email protected]>
Co-authored-by: Michael Small <[email protected]>
Co-authored-by: Michael Small <[email protected]>
| resource.value as Signal<readonly E[] | E[] | undefined>, | ||
| ); | ||
|
|
||
| /** |
There was a problem hiding this comment.
@wolfmanfx that's is for you. I had to make some impactful changes because the version before was directly using the original resource's value and not the proxied one, which means ids and entityMap would throw but not value
| } | ||
|
|
||
| return { idsLinked, entityMapLinked, entitiesSignal }; | ||
| function createComputedEntities<E extends Entity>( |
There was a problem hiding this comment.
was necessary to have two functions because of the later access resource's value
| const linkedState: Record<string, Signal<unknown>> = {}; | ||
| const computedProps: Record<string, Signal<unknown>> = {}; | ||
| const stateFactories = keys.map((name) => { | ||
| return (store: Record<string, unknown>) => { |
There was a problem hiding this comment.
I'll burn in the TypeScript hell for what I've done here 🔥😈
| string, | ||
| ResourceRef<readonly unknown[] | unknown[] | undefined> | ||
| >; | ||
| type Entity = { id: EntityId }; |
There was a problem hiding this comment.
these additional type short the code above
Replaces the computed linkedSignal with the direct resource value, using a Proxy on the get trap to ensure fail-safe access.