diff --git a/libs/ngrx-toolkit/src/lib/with-resource.spec.ts b/libs/ngrx-toolkit/src/lib/with-resource.spec.ts index 82e6b2d5..6c0aaa02 100644 --- a/libs/ngrx-toolkit/src/lib/with-resource.spec.ts +++ b/libs/ngrx-toolkit/src/lib/with-resource.spec.ts @@ -14,7 +14,7 @@ import { withState, } from '@ngrx/signals'; import { of } from 'rxjs'; -import { mapToResource, withResource } from './with-resource'; +import { ErrorHandling, mapToResource, withResource } from './with-resource'; describe('withResource', () => { describe('standard tests', () => { @@ -46,20 +46,23 @@ describe('withResource', () => { } } - function setupWithUnnamedResource() { + function setupWithUnnamedResource(errorHandling: ErrorHandling) { const addressResolver = { resolve: jest.fn(() => Promise.resolve(venice)), }; const AddressStore = signalStore( { providedIn: 'root', protectedState: false }, withState({ id: undefined as number | undefined }), - withResource((store) => { - const resolver = inject(AddressResolver); - return resource({ - params: store.id, - loader: ({ params: id }) => resolver.resolve(id), - }); - }), + withResource( + (store) => { + const resolver = inject(AddressResolver); + return resource({ + params: store.id, + loader: ({ params: id }) => resolver.resolve(id), + }); + }, + { errorHandling }, + ), withMethods((store) => ({ reload: () => store._reload() })), ); @@ -77,7 +80,7 @@ describe('withResource', () => { return { store, addressResolver }; } - function setupWithNamedResource() { + function setupWithNamedResource(errorHandling: ErrorHandling) { const addressResolver = { resolve: jest.fn(() => Promise.resolve(venice)), }; @@ -85,15 +88,18 @@ describe('withResource', () => { const UserStore = signalStore( { providedIn: 'root', protectedState: false }, withState({ id: undefined as number | undefined }), - withResource((store) => { - const resolver = inject(AddressResolver); - return { - address: resource({ - params: store.id, - loader: ({ params: id }) => resolver.resolve(id), - }), - }; - }), + withResource( + (store) => { + const resolver = inject(AddressResolver); + return { + address: resource({ + params: store.id, + loader: ({ params: id }) => resolver.resolve(id), + }), + }; + }, + { errorHandling }, + ), withMethods((store) => ({ reload: () => store._addressReload() })), ); @@ -185,34 +191,55 @@ describe('withResource', () => { }); describe('status checks', () => { - for (const { name, setup } of [ - { - name: 'unnamed resource', - setup: () => { - const { store, addressResolver } = setupWithUnnamedResource(); - const setId = (id: number) => patchState(store, { id }); - const setValue = (value: Address) => patchState(store, { value }); - return { - storeAndResource: store, - addressResolver, - setId, - setValue, - }; + describe.each([ + 'native', + 'undefined value', + 'previous value', + ] as ErrorHandling[])(`Error Handling: %s`, (errorHandling) => { + const testParameters = [ + { + name: 'unnamed resource', + setup: () => { + const { store, addressResolver } = + setupWithUnnamedResource(errorHandling); + const setId = (id: number) => patchState(store, { id }); + const setValue = (value: Address) => + patchState(store, { value }); + return { + storeAndResource: store, + addressResolver, + setId, + setValue, + }; + }, }, - }, - { - name: 'mapped named resource', - setup: () => { - const { store, addressResolver } = setupWithNamedResource(); - const storeAndResource = mapToResource(store, 'address'); - const setId = (id: number) => patchState(store, { id }); - const setValue = (value: Address) => - patchState(store, { addressValue: value }); - return { storeAndResource, addressResolver, setId, setValue }; + { + name: 'mapped named resource', + setup: () => { + const { store, addressResolver } = + setupWithNamedResource(errorHandling); + const storeAndResource = mapToResource(store, 'address'); + const setId = (id: number) => patchState(store, { id }); + const setValue = (value: Address) => + patchState(store, { addressValue: value }); + return { storeAndResource, addressResolver, setId, setValue }; + }, }, - }, - ]) { - describe(name, () => { + { + name: 'mapped named resource', + setup: () => { + const { store, addressResolver } = + setupWithNamedResource(errorHandling); + const storeAndResource = mapToResource(store, 'address'); + const setId = (id: number) => patchState(store, { id }); + const setValue = (value: Address) => + patchState(store, { addressValue: value }); + return { storeAndResource, addressResolver, setId, setValue }; + }, + }, + ]; + + describe.each(testParameters)(`$name`, ({ setup }) => { it('has idle status in the beginning', () => { const { storeAndResource } = setup(); @@ -251,20 +278,6 @@ describe('withResource', () => { expect(storeAndResource.hasValue()).toBe(true); }); - it('has error status when error', async () => { - const { storeAndResource, addressResolver, setId } = setup(); - - addressResolver.resolve.mockRejectedValue(new Error('Error')); - setId(1); - await wait(); - - expect(storeAndResource.status()).toBe('error'); - expect(() => storeAndResource.value()).toThrow(); - expect(storeAndResource.error()).toBeInstanceOf(Error); - expect(storeAndResource.isLoading()).toBe(false); - expect(storeAndResource.hasValue()).toBe(false); - }); - it('has local once updated', async () => { const { storeAndResource, addressResolver, setId, setValue } = setup(); @@ -282,109 +295,139 @@ describe('withResource', () => { expect(storeAndResource.hasValue()).toBe(true); }); }); - } - it('reloads an unnamed resource', async () => { - const { store, addressResolver } = setupWithUnnamedResource(); + it('reloads an unnamed resource', async () => { + const { store, addressResolver } = + setupWithUnnamedResource(errorHandling); - addressResolver.resolve.mockResolvedValue(venice); - patchState(store, { id: 1 }); + addressResolver.resolve.mockResolvedValue(venice); + patchState(store, { id: 1 }); - await wait(); - expect(store.hasValue()).toBe(true); + await wait(); + expect(store.hasValue()).toBe(true); + + addressResolver.resolve.mockResolvedValue({ + ...venice, + country: 'Great Britain', + }); + store.reload(); - addressResolver.resolve.mockResolvedValue({ - ...venice, - country: 'Great Britain', + await wait(); + expect(store.value()?.country).toBe('Great Britain'); }); - store.reload(); - await wait(); - expect(store.value()?.country).toBe('Great Britain'); - }); + describe('named resource', () => { + it('has idle status in the beginning', () => { + const { store } = setupWithNamedResource(errorHandling); - describe('named resource', () => { - it('has idle status in the beginning', () => { - const { store } = setupWithNamedResource(); + expect(store.addressStatus()).toBe('idle'); + expect(store.addressValue()).toBeUndefined(); + expect(store.addressError()).toBeUndefined(); + expect(store.addressIsLoading()).toBe(false); + expect(store.addressHasValue()).toBe(false); + }); - expect(store.addressStatus()).toBe('idle'); - expect(store.addressValue()).toBeUndefined(); - expect(store.addressError()).toBeUndefined(); - expect(store.addressIsLoading()).toBe(false); - expect(store.addressHasValue()).toBe(false); - }); + it('has loading status when loading', () => { + const { store, addressResolver } = + setupWithNamedResource(errorHandling); - it('has loading status when loading', () => { - const { store, addressResolver } = setupWithNamedResource(); + addressResolver.resolve.mockResolvedValue(venice); + patchState(store, { id: 1 }); - addressResolver.resolve.mockResolvedValue(venice); - patchState(store, { id: 1 }); + expect(store.addressStatus()).toBe('loading'); + expect(store.addressValue()).toBeUndefined(); + expect(store.addressError()).toBeUndefined(); + expect(store.addressIsLoading()).toBe(true); + expect(store.addressHasValue()).toBe(false); + }); - expect(store.addressStatus()).toBe('loading'); - expect(store.addressValue()).toBeUndefined(); - expect(store.addressError()).toBeUndefined(); - expect(store.addressIsLoading()).toBe(true); - expect(store.addressHasValue()).toBe(false); - }); + it('has resolved status when loaded', async () => { + const { store, addressResolver } = + setupWithNamedResource(errorHandling); - it('has resolved status when loaded', async () => { - const { store, addressResolver } = setupWithNamedResource(); + addressResolver.resolve.mockResolvedValue(venice); + patchState(store, { id: 1 }); - addressResolver.resolve.mockResolvedValue(venice); - patchState(store, { id: 1 }); + await wait(); - await wait(); + expect(store.addressStatus()).toBe('resolved'); + expect(store.addressValue()).toEqual(venice); + expect(store.addressError()).toBeUndefined(); + expect(store.addressIsLoading()).toBe(false); + expect(store.addressHasValue()).toBe(true); + }); - expect(store.addressStatus()).toBe('resolved'); - expect(store.addressValue()).toEqual(venice); - expect(store.addressError()).toBeUndefined(); - expect(store.addressIsLoading()).toBe(false); - expect(store.addressHasValue()).toBe(true); - }); + it('has error status when error', async () => { + const { store, addressResolver } = + setupWithNamedResource(errorHandling); - it('has error status when error', async () => { - const { store, addressResolver } = setupWithNamedResource(); + addressResolver.resolve.mockRejectedValue(new Error('Error')); + patchState(store, { id: 1 }); + await wait(); - addressResolver.resolve.mockRejectedValue(new Error('Error')); - patchState(store, { id: 1 }); - await wait(); + expect(store.addressStatus()).toBe('error'); + expect(() => store.addressValue()).toThrow(); + expect(store.addressError()).toBeInstanceOf(Error); + expect(store.addressIsLoading()).toBe(false); + expect(store.addressHasValue()).toBe(false); + }); - expect(store.addressStatus()).toBe('error'); - expect(() => store.addressValue()).toThrow(); - expect(store.addressError()).toBeInstanceOf(Error); - expect(store.addressIsLoading()).toBe(false); - expect(store.addressHasValue()).toBe(false); - }); + it('has local once updated', async () => { + const { store, addressResolver } = + setupWithNamedResource(errorHandling); - it('has local once updated', async () => { - const { store, addressResolver } = setupWithNamedResource(); + addressResolver.resolve.mockResolvedValue(venice); + patchState(store, { id: 1 }); - addressResolver.resolve.mockResolvedValue(venice); - patchState(store, { id: 1 }); + await wait(); + patchState(store, ({ addressValue }) => ({ + addressValue: addressValue + ? { ...addressValue, country: 'Italia' } + : undefined, + })); + + expect(store.addressStatus()).toBe('local'); + expect(store.addressValue()?.country).toBe('Italia'); + expect(store.addressError()).toBeUndefined(); + expect(store.addressIsLoading()).toBe(false); + expect(store.addressHasValue()).toBe(true); + }); - await wait(); - patchState(store, ({ addressValue }) => ({ - addressValue: addressValue - ? { ...addressValue, country: 'Italia' } - : undefined, - })); - - expect(store.addressStatus()).toBe('local'); - expect(store.addressValue()?.country).toBe('Italia'); - expect(store.addressError()).toBeUndefined(); - expect(store.addressIsLoading()).toBe(false); - expect(store.addressHasValue()).toBe(true); + it('can also reload by resource name', async () => { + const { store, addressResolver } = + setupWithNamedResource(errorHandling); + + addressResolver.resolve.mockResolvedValueOnce(venice); + patchState(store, { id: 1 }); + await wait(); + expect(store.addressStatus()).toBe('resolved'); + store.reload(); + expect(store.addressStatus()).toBe('reloading'); + }); }); + }); + + describe('error status', () => { + describe('unnamed resource', () => { + it('throws on error with native error handling', async () => { + const { store, addressResolver } = + setupWithUnnamedResource('native'); - it('can also reload by resource name', async () => { - const { store, addressResolver } = setupWithNamedResource(); + addressResolver.resolve.mockRejectedValue(new Error('Error')); + patchState(store, { id: 1 }); + await wait(); + + expect(() => store.value()).toThrow(); + }); + }); + it('returns undefined with undefined value error handling', async () => { + const { store, addressResolver } = + setupWithUnnamedResource('undefined value'); - addressResolver.resolve.mockResolvedValueOnce(venice); + addressResolver.resolve.mockRejectedValue(new Error('Error')); patchState(store, { id: 1 }); await wait(); - expect(store.addressStatus()).toBe('resolved'); - store.reload(); - expect(store.addressStatus()).toBe('reloading'); + expect(store.value()).toBeUndefined(); }); }); }); @@ -396,16 +439,10 @@ describe('withResource', () => { warningSpy.mockClear(); }); - for (const memberName of [ - //TODO wait for https://github.com/ngrx/platform/pull/4932 - // 'value', - 'status', - 'error', - 'isLoading', - '_reload', - 'hasValue', - ]) { - it(`warns if ${memberName} is not a member of the store`, () => { + //TODO wait for https://github.com/ngrx/platform/pull/4932 and then add 'value' to the list + it.each(['status', 'error', 'isLoading', '_reload', 'hasValue'])( + `warns if %s is not a member of the store`, + (memberName) => { const Store = signalStore( { providedIn: 'root' }, withProps(() => ({ [memberName]: true })), @@ -421,8 +458,8 @@ describe('withResource', () => { 'Trying to override:', memberName, ); - }); - } + }, + ); //TODO wait for https://github.com/ngrx/platform/pull/4932 it.skip('also checks for named resources', () => { diff --git a/libs/ngrx-toolkit/src/lib/with-resource.ts b/libs/ngrx-toolkit/src/lib/with-resource.ts index c2011b29..c81d3198 100644 --- a/libs/ngrx-toolkit/src/lib/with-resource.ts +++ b/libs/ngrx-toolkit/src/lib/with-resource.ts @@ -2,6 +2,7 @@ import { isSignal, + linkedSignal, Resource, ResourceRef, ResourceStatus, @@ -33,10 +34,17 @@ export type ResourceResult = { export type ResourceDictionary = Record>; -export type NamedResourceResult = { +export type NamedResourceResult< + T extends ResourceDictionary, + UndefinedErrorHandling extends boolean, +> = { state: { [Prop in keyof T as `${Prop & - string}Value`]: T[Prop]['value'] extends Signal ? S : never; + string}Value`]: T[Prop]['value'] extends Signal + ? UndefinedErrorHandling extends true + ? S | undefined + : S + : never; }; props: { [Prop in keyof T as `${Prop & string}Status`]: Signal; @@ -54,6 +62,16 @@ export type NamedResourceResult = { }; }; +export type ErrorHandling = 'native' | 'undefined value' | 'previous value'; + +export type ResourceOptions = { + errorHandling?: ErrorHandling; +}; + +const defaultOptions: Required = { + errorHandling: 'undefined value', +}; + //** Implementation of `withResource` */ /** @@ -63,7 +81,7 @@ export type NamedResourceResult = { * Integrates a `Resource` into the SignalStore and makes the store instance * implement the `Resource` interface. * - * The resource’s value is stored under the `value` key in the state + * The resource's value is stored under the `value` key in the state * and is exposed as a `DeepSignal`. * * It can also be updated via `patchState`. @@ -85,7 +103,8 @@ export type NamedResourceResult = { * ``` * * @param resourceFactory A factory function that receives the store's state signals, - * methods, and props. Needs to return a `ResourceRef`. + * methods, and props. + * @param resourceOptions Allows to configure the error handling behavior. */ export function withResource< Input extends SignalStoreFeatureResult, @@ -94,6 +113,17 @@ export function withResource< resourceFactory: ( store: Input['props'] & Input['methods'] & StateSignals, ) => ResourceRef, + resourceOptions: { errorHandling: 'undefined value' }, +): SignalStoreFeature>; + +export function withResource< + Input extends SignalStoreFeatureResult, + ResourceValue, +>( + resourceFactory: ( + store: Input['props'] & Input['methods'] & StateSignals, + ) => ResourceRef, + resourceOptions?: ResourceOptions, ): SignalStoreFeature>; /** @@ -128,6 +158,7 @@ export function withResource< * * @param resourceFactory A factory function that receives the store's props, * methods, and state signals. It must return a `Record`. + * @param resourceOptions Allows to configure the error handling behavior. */ export function withResource< Input extends SignalStoreFeatureResult, @@ -136,7 +167,18 @@ export function withResource< resourceFactory: ( store: Input['props'] & Input['methods'] & StateSignals, ) => Dictionary, -): SignalStoreFeature>; + resourceOptions: { errorHandling: 'undefined value' }, +): SignalStoreFeature>; + +export function withResource< + Input extends SignalStoreFeatureResult, + Dictionary extends ResourceDictionary, +>( + resourceFactory: ( + store: Input['props'] & Input['methods'] & StateSignals, + ) => Dictionary, + resourceOptions?: ResourceOptions, +): SignalStoreFeature>; export function withResource< Input extends SignalStoreFeatureResult, @@ -145,7 +187,12 @@ export function withResource< resourceFactory: ( store: Input['props'] & Input['methods'] & StateSignals, ) => ResourceRef | ResourceDictionary, + resourceOptions?: ResourceOptions, ): SignalStoreFeature { + const options: Required = { + ...defaultOptions, + ...(resourceOptions || {}), + }; return (store) => { const resourceOrDictionary = resourceFactory({ ...store.stateSignals, @@ -154,22 +201,31 @@ export function withResource< }); if (isResourceRef(resourceOrDictionary)) { - return createUnnamedResource(resourceOrDictionary)(store); + return createUnnamedResource( + resourceOrDictionary, + options.errorHandling, + )(store); } else { - return createNamedResource(resourceOrDictionary)(store); + return createNamedResource( + resourceOrDictionary, + options.errorHandling, + )(store); } }; } function createUnnamedResource( resource: ResourceRef, + errorHandling: ErrorHandling, ) { function hasValue(): this is Resource> { return resource.hasValue(); } return signalStoreFeature( - withLinkedState(() => ({ value: resource.value })), + withLinkedState(() => ({ + value: valueSignalForErrorHandling(resource, errorHandling), + })), withProps(() => ({ status: resource.status, error: resource.error, @@ -184,13 +240,17 @@ function createUnnamedResource( function createNamedResource( dictionary: Dictionary, + errorHandling: ErrorHandling, ) { const keys = Object.keys(dictionary); const state: Record> = keys.reduce( (state, resourceName) => ({ ...state, - [`${resourceName}Value`]: dictionary[resourceName].value, + [`${resourceName}Value`]: valueSignalForErrorHandling( + dictionary[resourceName], + errorHandling, + ), }), {}, ); @@ -319,3 +379,113 @@ export function mapToResource< hasValue, } as MappedResource; } + +/** + * Strategies to work around the error throwing behavior of the Resource API. + * + * The original idea was to use a `linkedSignal` as the state's value Signal. It would mean + * that we can leverage the `computation` callback to handle the error. The downside is that + * changes to that signal will not be reflected in the underlying resource, i.e. the resource + * will not switch to status `local`. + * + * 1. An option to fix that would be to put the `linkedSignal` as property in the SignalStore, + * where it would have the name `value`. Given, we apply a `DeepSignal` to it, it would not + * break from the outside. The original value would be put into the state behind a hidden Symbol + * as property name. In order to update the state, users will get an updater function, called + * `setResource`. + * + * That works perfectly for unnamed resources, but could cause potential problems + * for named resources, when they are defined multiple times, i.e. calling `withResource` + * multiple times. The reason is that, we would have to hide their values in the state again + * behind a symbol, but that would be a property which gets defined once, and would get new + * subproperties (the values of the resources) added per additional `withResource` call. + * + * Using a separate updated method is a common SignalStore pattern, which is also used + * in `withEntities`. + * + * For named resources, `setResource` would come with a name as first parameter. + * + * We saw in earlier experiments that there are TypeScript-specific challenges. + * + * Pros: + * - Uses Angular's native `linkedSignal` and isn't a hackish approach + * - Status transitions to 'local' work correctly (via direct `res.value.set()` in `setResource`) + * - Works with `patchState`/`getState` (linkedSignal handles errors on read) + * - Clear, explicit API with dedicated `setResource()` method + * + * Cons: + * - Requires API change: users must use `setResource()` instead of `patchState(store, { value })` + * - Named resources with multiple `withResource` calls: hidden state management becomes complex + * + * 2. A possible alternative would be to use a Proxy on value. Instead of using a `linkedSignal`, + * we can leave the value signal as is and create a proxy around it that intercepts the get/call + * operation and handles the error. The downside is that we need to implement the proxy ourselves, + * which is not as clean as using a `linkedSignal`. On the other hand, the Angular team is working + * on a better way to handle errors, which means that approach is only temporary. It could also + * happen, that we are getting some sort of "Mapped Signal", where not just the reading (as in + * `linkedSignal`) but also the writing is handled. + * + * Pros: + * - No API changes: `patchState(store, { value: x })` works naturally + * - Status transitions to 'local' work correctly (writes go directly to original signal) + * - Works with `patchState`/`getState` (proxy intercepts reads and handles errors) + * - Uniform solution: same approach for both named and unnamed resources + * - Transparent: looks and behaves like a normal signal from the outside + * + * Cons: + * - Manual implementation: must properly handle all signal methods (`set`, `update`, `asReadonly`, etc.) + * - Dependency tracking: need to verify proxy doesn't break Angular's reactivity system + * - More complex proxy logic required for 'previous value' strategy (caching previous values) + * - Less "Angular-native": doesn't leverage `linkedSignal`'s built-in reactivity guarantees + */ + +// We require an explicit error symbol, to clearly communicate +// that an error has happened. Since a resource's value can be +// any value, we're ensuring here that an error of true is not +// mistakenly seen as value. +const ERROR = Symbol('ERROR'); + +function valueSignalForErrorHandling( + res: ResourceRef, + errorHandling: 'undefined value', +): WritableSignal; + +function valueSignalForErrorHandling( + res: ResourceRef, + errorHandling: ErrorHandling, +): WritableSignal; + +function valueSignalForErrorHandling( + res: ResourceRef, + errorHandling: ErrorHandling, +): WritableSignal { + switch (errorHandling) { + case 'native': + return res.value; + case 'undefined value': + return linkedSignal(() => + res.status() === 'error' ? undefined : res.value(), + ); + case 'previous value': + return linkedSignal({ + source: () => { + if (res.status() === 'error') { + return ERROR; + } + return res.value(); + }, + computation: (source, previous) => { + if (source === ERROR) { + if (previous === undefined) { + throw new Error( + 'impossible state: previous value is not available -> resource was initialized with error', + ); + } + return previous.value; + } else { + return source; + } + }, + }); + } +}