ngrx/platform

Bug: using nested signalStoreFeature causes an error when applying mapped-types over generics in withComputed

Closed this issue · 10 comments

SerkanSipahi commented

Which @ngrx/* package(s) are the source of the bug?

signals

Minimal reproduction of the bug/regression with instructions

https://stackblitz.com/edit/github-4qdexu-whtv54?file=src%2Fmain.ts,src%2Findex.html (see main.ts)

Current behavior:

Nested signalStoreFeatures are not a concern, such as using signalStoreFeature within signalStoreFeature. However, an error occurs in signalStoreFeatureParent when using withFeatureStoreChild and MappedTypeOverGenericUnions along with withState(...) or withEntities(...) in signalStoreFeatureParent, see return () => signalStoreFeature(withFeatureStoreChild(keys));, see minimal stackblitz reproduction or below the code example.

Some observations:

  • when removing withState and withEntities and MappedTypeOverGenericUnions<Key> is present the error disappears
  • error accours only when mapped-types are used which receive their types over generics, see MappedTypeOverGenericUnions<Key>
  • if withState and withEntities are preserved and MappedTypeOverStaticUnions is used then there is no error.

Example:

import {
  signalStoreFeature,
  type,
  withComputed,
  withState,
} from '@ngrx/signals';
import { withEntities } from '@ngrx/signals/entities';
import { Signal } from '@angular/core';

type DictOverMappedGenericType<Key extends string> = RecordFoo &
  MappedTypeOverGenericUnions<Key>;

type DictOverMappedStaticType = RecordFoo & MappedTypeOverStaticUnions;

type RecordFoo = Record<'foo', Signal<'foo'>>;

// ❌ @ngrx/signal cannot handle generics types (see Key extends string) that are used in mapped-types.
type MappedTypeOverGenericUnions<Key extends string> = {
  [Value in Key as `foo${Capitalize<Value>}`]: Signal<Value>;
};

// ✅ (works)
type MappedTypeOverStaticUnions = {
  [Value in 'a' | 'b' as `foo${Capitalize<Value>}`]: Signal<Value>;
};

function withFeatureStoreChild<Key extends string>(ids: Key[]) {
  return signalStoreFeature(
    withState({ foo: '' }),
    withEntities({ entity: type<{ id: number; name: string }>() }),
    // withComputed((store) => ({} as DictOverMappedStaticType)) // ✅ (works, no error in `withFeatureStoreParent`)
    withComputed((store) => ({} as DictOverMappedGenericType<Key>)) // ❌ (using this fails in `withFeatureStoreParent`)
  );
}

function withFeatureStoreParent<
  Model extends Record<string, unknown>,
  Key extends keyof Model & string
>(keys: Key[]) {
  // ❌ The typescript error happens here (`withFeatureStoreChild`) when `DictOverMappedGenericType<Key>` is used in `withFeatureStoreChild.withComputed` and
  // when one of `withState` or `withEntities` is present in `withFeatureStoreChild`
  return () => signalStoreFeature(withFeatureStoreChild(keys)); // ❌ Error
}

Expected behavior

My expectation is that the use of MappedTypeOverGenericUnions<Key> does not lead to an error in withFeatureStoreParent. Mapped types in general do not lead to an error, see MappedTypeOverStaticUnions in withFeatureStoreChild.

Versions of NgRx, Angular, Node, affected browser(s) and operating system(s)

@ngrx/signals 18.1.0

Other information

I assume that this is an error and must be rectified. Is there a workaround in the meantime?

I would be willing to submit a PR to fix this issue

  • Yes
  • No
rainerhahnekamp commented

Ouch, once I saw your name, I knew that one isn't going to be easy ;)

Thanks for the stackblitz.

I have no idea. I tried out various combinations. One could investigate setting the types for withComputed, but I think that this will not work either.

Hopefully someone else from the team can help you out here.

  • 👀1
SerkanSipahi commented

I found a workaround. When adding{ [key: string]: Signal<unknown> } to DictOverMappedGenericType<Key extends string> I was able to fix the error. I know doing this opens a wildcard for accessing properties but it is only a small disadvantage. If someone uses the wildcard to attempt access to a property, the result will be unknown (Signal) and that's fine for my scenarios. Thank you @rainerhahnekamp for trying to help me.

New Stackblitz https://stackblitz.com/edit/github-4qdexu-yasxch?file=src%2Fmain.ts,src%2Findex.html

type DictOverMappedGenericType<Key extends string> = 
  RecordFoo &
  MappedTypeOverGenericUnions<Key> & // This generic type within a mapped-typed led to an error but the magic line below was able to fix the error
  { [key: string]: Signal<unknown> }; // 🪄 this is the magic line

⁉️ Nevertheless, I wonder whether my initial issue report is a bug?

rainerhahnekamp commented

Congratulations on finding the fix yourself! 🎉

⁉️ Nevertheless, I wonder whether my initial issue report is a bug?

🤔 It’s hard to say. Especially with signalStoreFeature, we’re really pushing the boundaries of TypeScript. This might be worth considering for the documentation, though there’s a concern that documenting it could lead to overuse.

I see your example as a highly advanced use case. For scenarios like this, it might be best for developers like you to open an issue when encountering similar challenges. That way, we can track these edge cases and hopefully remember approaches like yours in the future.

  • 👍2
rainerhahnekamp commented

@SerkanSipahi, as discussed above, I’ll go ahead and close this for now. If anything else comes up, feel free to reopen it.

Thanks again for providing the solution!

  • 👍1
SerkanSipahi commented

@rainerhahnekamp

I want to say something in conclusion. For the average developer this can be seen as an advanced use case but for a library developer, this case is common. Basically, I just tried to pass a generic type through several nested signalStoreFeature functions. Nothing special actually. It can look complicated or complex but it only looks like this because it is in the context of signalStoreFeature. It was just a small snapshot of my current work.

rainerhahnekamp commented

@SerkanSipahi Yes, we might want to add a section for library developers that covers this and other cases.

  • 👍1
SerkanSipahi commented

@markostanimirovic Thanks I will test it. Did you fix it or is it just gone?

markostanimirovic commented

It's (probably) fixed in the commit for adding the withProps feature.

  • 👍1
SerkanSipahi commented

@markostanimirovic error gone in @ngrx/signals@19.0.0-rc.0 thanks.