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

fix(signals): improve state type and add type tests #4064

Merged
merged 4 commits into from
Oct 18, 2023

Conversation

markostanimirovic
Copy link
Member

@markostanimirovic markostanimirovic commented Oct 14, 2023

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

Closes #4065

  1. DeepSignal result is not prettified:
const state = signalState({ user: { firstName: 'Marko' } });

const user = state.user; // type: DeepSignal<{ firstName: string }>
const firstName = user.firstName; // type: DeepSignal<string>
  1. The state can contain function properties (name, bind, call, apply, etc.) which will throw a runtime error because signal function properties cannot be overridden.

  2. Deep signals for unknown dictionaries are accidentally created:

type State = { foo: Record<string, { bar: number }> };
const state = signalState<State>({ foo: {} });

const foo = state.foo; // type: DeepSignal<Record<string, { bar: number }>
const foo = state.foo['asdf'] // type: DeepSignal<{ bar: number }>
  1. Signal store allows the unknown dictionary to be the state type:
const Store = signalStore(
  withState<Record<string, { foo: number }>>({})
);

const store = new Store();
const x = store['x']; // type: DeepSignal<{ foo: number }>
const y = store['y']; // type: DeepSignal<{ foo: number }>

What is the new behavior?

  1. DeepSignal result is prettified:
const state = signalState({ user: { firstName: 'Marko' } });

const user = state.user; // type: DeepSignal<{ firstName: string }>
const firstName = user.firstName; // type: Signal<string> 👈
  1. The state cannot contain function properties:
// compilation error: @ngrx/signals: signal state properties must be different from `Function` properties
const state = signalState({ bind: 'Marko' });
  1. Deep signals for unknown dictionaries are not accidentally created:
type State = { foo: Record<string, { bar: number }> };
const state = signalState<State>({ foo: {} });

const foo = state.foo; // type: Signal<Record<string, { bar: number }>
const foo = state.foo['asdf'] // compilation error
  1. Signal store doesn't allow the unknown dictionary to be the state type:
const Store = signalStore(
  // compilation error: @ngrx/signals: root state keys must be string literals
  withState<Record<string, { foo: number }>>({})
);

const store = new Store();
const x = store['x']; // type: DeepSignal<{ foo: number }> // compilation error
const y = store['y']; // type: DeepSignal<{ foo: number }> // compilation error

Does this PR introduce a breaking change?

[ ] Yes
[x] No

@netlify
Copy link

netlify bot commented Oct 14, 2023

Deploy Preview for ngrx-io canceled.

Name Link
🔨 Latest commit a0ee53b
🔍 Latest deploy log https://app.netlify.com/sites/ngrx-io/deploys/652f01c056d41c00089809c5

@markostanimirovic markostanimirovic changed the title fix(signals): fix state type and add type tests fix(signals): improve state type and add type tests Oct 14, 2023
@markostanimirovic markostanimirovic marked this pull request as draft October 15, 2023 23:42
@markostanimirovic markostanimirovic marked this pull request as ready for review October 17, 2023 10:06
modules/signals/src/signal-state.ts Outdated Show resolved Hide resolved
modules/signals/src/with-state.ts Outdated Show resolved Hide resolved

export type HasFunctionKeys<T> = T extends Record<string, unknown>
? {
[K in keyof T]: K extends keyof Function ? true : HasFunctionKeys<T[K]>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👀 this is smart

@timdeschryver timdeschryver merged commit 10c93ed into main Oct 18, 2023
5 checks passed
@timdeschryver timdeschryver deleted the test/signals/type-tests branch October 18, 2023 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

@ngrx/signals: Add type tests
3 participants