ngxtension / ngxtension-platform

Utilities for Angular
https://ngxtension.netlify.app/
MIT License
592 stars 87 forks source link

toLazySignal broken with Angular 18.1.x #474

Open SpeedoPasanen opened 2 months ago

SpeedoPasanen commented 2 months ago

ToSignalOptions in @angular/core/rxjs-interop now requires a generic type, which breaks toLazySignal because it's options doesn't have a type.

Only workaround I can think of is to roll back to Angular 18.0.x

eneajaho commented 2 months ago

or we just add a generic type too

ShacharHarshuv commented 1 month ago

Typing a generic type doesn't always work.

for example:

toLazySignal<string>(..., {
  requireSync: true
});

Infers as Signal<string | undefined> incorrectly. (Should be Signal<string>). Extracting the options to a separate variable with an explicit generic also doesn't work here.

I believe this update was caused by a typescript update. Looking at Angular's code, they changed the type definitions of toSignal to use NoInfer and pass a generic to options. If you do the same changes for toLazySignal it works:

export declare function toLazySignal<T>(source: Observable<T> | Subscribable<T>): Signal<T | undefined>;
export declare function toLazySignal<T>(source: Observable<T> | Subscribable<T>, options: NoInfer<ToSignalOptions<T | undefined> & {
    initialValue?: undefined;
    requireSync?: false;
}>): Signal<T | undefined>;
export declare function toLazySignal<T>(source: Observable<T> | Subscribable<T>, options: NoInfer<ToSignalOptions<T | null> & {
    initialValue?: null;
    requireSync?: false;
}>): Signal<T | null>;
export declare function toLazySignal<T>(source: Observable<T> | Subscribable<T>, options: NoInfer<ToSignalOptions<T> & {
    initialValue?: undefined;
    requireSync: true;
}>): Signal<T>;
export declare function toLazySignal<T, const U extends T>(source: Observable<T> | Subscribable<T>, options: NoInfer<ToSignalOptions<T | U> & {
    initialValue: U;
    requireSync?: false;
}>): Signal<T | U>;

So a better workaround than rolling back would be using patch-package with these changes, or creating your local copy of toLazySignal. Hopefully you can revert that once it gets fixed in ngxensions.

FYI, NoInfer was introduced in typescript 5.4, so that would mean ngxtension would have to add that as a minimum verison.

eneajaho commented 1 month ago

cc @e-oz

e-oz commented 1 month ago

I'm on it

e-oz commented 1 month ago

as soon as ngx-tension update its deps to 18.1+, tests in this PR will stop failing: https://github.com/ngxtension/ngxtension-platform/pull/484#issuecomment-2289723478