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): allow using signalStore and signalState in TS libs #4152

Merged
merged 1 commit into from
Nov 27, 2023

Conversation

markostanimirovic
Copy link
Member

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 #4151

What is the new behavior?

signalStore and signalState can be used/exported in projects that have tsconfig with "declaration": true.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

This is a workaround for the following TS bug: microsoft/TypeScript#37888

Copy link

netlify bot commented Nov 24, 2023

Deploy Preview for ngrx-io canceled.

Name Link
🔨 Latest commit 0dd033a
🔍 Latest deploy log https://app.netlify.com/sites/ngrx-io/deploys/6560e3d5d77a7500095fe488

Comment on lines 19 to +22
Prettify<
SignalStoreSlices<FeatureResult['state']> &
FeatureResult['signals'] &
FeatureResult['methods'] &
SignalStateMeta<Prettify<FeatureResult['state']>>
FeatureResult['methods']
Copy link
Member Author

Choose a reason for hiding this comment

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

A workaround for this TS bug is to always put a unique symbol inside of the named type.

Before:

The STATE_SIGNAL was appended to the dynamic SignalStore type by prettifying the intersection.

const CounterStore = signalStore(withState({ count: 0 }));
// type: Type<{ count: Signal<number>; [STATE_SIGNAL]: WritableSignal<{ count: number }> }>

After:

The STATE_SIGNAL is always exported as a part of the StateSignal type.

const CounterStore = signalStore(withState({ count: 0 }));
// type: Type<{ count: Signal<number>; } & StateSignal<{ count: number }>>

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense.

StateSignal has to be added to the public APIs index.ts as well, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not needed. We can keep it private.

@@ -8,7 +8,7 @@ import { IsKnownRecord } from './ts-helpers';

// An extended Signal type that enables the correct typing
// of nested signals with the `name` or `length` key.
interface Signal<T> extends NgSignal<T> {
export interface Signal<T> extends NgSignal<T> {
Copy link
Member Author

Choose a reason for hiding this comment

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

This export fixes the same compilation error in TS libraries with declaration: true when signalState is used. The problem was caused by another unique symbol - Angular's SIGNAL.

Copy link
Member

Choose a reason for hiding this comment

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

👍

@otkrivashkin
Copy link

Please, somebody approve it)

@brandonroberts brandonroberts merged commit ecc247c into main Nov 27, 2023
5 checks passed
@brandonroberts brandonroberts deleted the fix/signals/ts-libs branch November 27, 2023 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants