platform icon indicating copy to clipboard operation
platform copied to clipboard

createAction() does not handle generic types correctly

Open tomer953 opened this issue 2 years ago • 3 comments

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

store

Minimal reproduction of the bug/regression with instructions

import { createAction, props } from '@ngrx/store';
import { PcDataKey, PcDataType, RequestStatus } from './state.interface';

export const setUsername = createAction(
  '[Main] setUsername',
  props<{ username: string }>()
);
export const setPcStatus = createAction(
  '[Main] setPcStatus',
  props<{ key: PcDataKey; status: RequestStatus }>()
);

// props does not support for generic types, so I used the arrow function method:
const pcDataProps = <K extends PcDataKey>(obj: {
  key: K;
  data: PcDataType<K>;
}) => obj;
export const setPcData = createAction('[Main] setPcData', pcDataProps);

// now, while this throws as it should (type safety for the data type based on the key):
pcDataProps({ key: 'Drives', data: { response: 100 } });
// this doesn't, so the createAction is somewhere ignore the generic types:
setPcData({ key: 'Drives', data: { response: 100 } }); // Should throw Type '{ response: number; }' is not assignable to type 'DrivesResult'

Full code, interfaces and such are here:

https://stackblitz.com/github/tomer953/ngrx-store-generic-types?file=src%2Fapp%2Fstore%2Factions.ts

Expected behavior

assume we have a function that use generic types to infer the arguments type, ie:

<K extends PcDataKey>(obj: {
  key: K;
  data: PcDataType<K>;
})

The createAction function should enforce strict typing based on the provided function, such that providing incorrect data types for data should result in a compilation error.

so the signature should look like:

image

but instead is:

image

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

Angular CLI: 16.2.9 Node: 20.9.0 (Unsupported) Package Manager: npm 10.1.0 OS: win32 x64

Angular: 16.2.12 ... animations, common, compiler, compiler-cli, core, forms ... platform-browser, platform-browser-dynamic, router

Package Version

@angular-devkit/architect 0.1602.9 @angular-devkit/build-angular 16.2.9 @angular-devkit/core 16.2.9 @angular-devkit/schematics 16.2.9 @angular/cli 16.2.9 @schematics/angular 16.2.9 rxjs 7.8.1 typescript 5.1.6 zone.js 0.13.3 @ngrx/store 16.3.0

Other information

No response

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

  • [ ] Yes
  • [ ] No

tomer953 avatar Nov 06 '23 13:11 tomer953

B.t.w, I wrapped with object obj since when I used a "flat" version, no type safety at all, so it can be another bug, not sure:

type PcDataItem<K extends PcDataKey> = { key: K; data: PcDataType<K> };
const pcDataPropsFlatten = <K extends PcDataKey>(key: K, data: PcDataType<K>) =>
  ({ key, data } as PcDataItem<K>);
pcDataPropsFlatten('Ping', []); // works as expected with type checking for the data
const setPcDataTest = createAction('[Main] setPcData', pcDataPropsFlatten);
setPcDataTest('foo123', 'bar456'); // no type checking at all, both key,data are 'any'

image

tomer953 avatar Nov 06 '23 15:11 tomer953

@tomer953 , try the following approach (playground):

const pcDataProps2 = <K extends PcDataKey>(
  obj: K extends PcDataKey ? { key: K; data: PcDataType<K> } : never
) => obj;
export const setPcData2 = createAction('[Main] setPcData', pcDataProps2);

pcDataProps2({ key: 'Drives', data: { response: 100 } }); // Invalid
pcDataProps2({ key: 'Ping', data: { response: 100 } }); // Valid

setPcData2({ key: 'Drives', data: { response: 100 } }); // Invalid
setPcData2({ key: 'Ping', data: { response: 100 } }); // Valid

To be fair, I'm not exactly sure why it works (or rather, why your original approach does not). Probably, has something to do with better type narrowing due to never branch in the conditional type? Would love to hear some thoughts on this.

alexkunin avatar Mar 17 '24 02:03 alexkunin

@tomer953 , try the following approach (playground):

const pcDataProps2 = <K extends PcDataKey>(
  obj: K extends PcDataKey ? { key: K; data: PcDataType<K> } : never
) => obj;
export const setPcData2 = createAction('[Main] setPcData', pcDataProps2);

pcDataProps2({ key: 'Drives', data: { response: 100 } }); // Invalid
pcDataProps2({ key: 'Ping', data: { response: 100 } }); // Valid

setPcData2({ key: 'Drives', data: { response: 100 } }); // Invalid
setPcData2({ key: 'Ping', data: { response: 100 } }); // Valid

To be fair, I'm not exactly sure why it works (or rather, why your original approach does not). Probably, has something to do with better type narrowing due to never branch in the conditional type? Would love to hear some thoughts on this.

Hm, my hunch is that your solution is introduce deference with a conditional type, making the generic type evaluated later. I think NoInfer introduced in TS 5.4 may solve this issue...

david-shortman avatar Mar 24 '24 00:03 david-shortman