Skip to content
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

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

Open
2 tasks
SerkanSipahi opened this issue Nov 19, 2024 · 6 comments

Comments

@SerkanSipahi
Copy link
Contributor

SerkanSipahi commented Nov 19, 2024

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
@SerkanSipahi SerkanSipahi changed the title Bug: using nested signalStoreFeature causes an error when applying mapped types to generics in withComputed Bug: using nested signalStoreFeature causes an error when applying mapped-types over generics in withComputed Nov 19, 2024
@rainerhahnekamp
Copy link
Contributor

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.

@SerkanSipahi
Copy link
Contributor Author

SerkanSipahi commented Nov 19, 2024

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
Copy link
Contributor

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.

@rainerhahnekamp
Copy link
Contributor

@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!

@SerkanSipahi
Copy link
Contributor Author

SerkanSipahi commented Nov 20, 2024

@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
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants