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

Swap computedFrom<Input, Output> to computedFrom<Output, Input>? #73

Closed
renatoaraujoc opened this issue Sep 19, 2023 · 13 comments
Closed

Comments

@renatoaraujoc
Copy link

Hello @nartc and @eneajaho,

After a talk with Enea on Twitter, I've shown that maybe we could improve computedFrom type inference.

Right now, we can't easily tell computedFrom what kind of Output type we want it to be unless we specify the Input params first.

Right now we need to do this:

someNumber = signal<number | null>(null);

computedNumberToSomething = computedFrom<[number | null], number>(
    [this.someNumber],
    pipe(map(([someNumber]) => someNumber ?? 0))
);

Could we have something like:

someNumber = signal<number | null>(null);

computedNumberToSomething = computedFrom<number>(
    [this.someNumber],
    pipe(map(([someNumber]) => someNumber ?? 0))
);

It would be needed to swap <Input, Output> to <Output, Input> in computedFrom.

Second feedback:

Create an overloaded function that also accepts a non readonly array, so we can:

someNumber = signal<number | null>(null);

computedNumberToSomething = computedFrom<number>(
    this.someNumber,
    pipe(map(([someNumber]) => someNumber ?? 0))
);

What yall think?

Renato

@nartc
Copy link
Collaborator

nartc commented Sep 20, 2023

Hey, the first feedback makes sense to me but I don't quite get the 2nd feedback. Can you elaborate a bit?

@tomer953
Copy link
Contributor

I think he wants to spread the args
ie in addition to computedFrom([a,b,c])
also support computedFrom(a,b,c)

@nartc
Copy link
Collaborator

nartc commented Sep 20, 2023

@tomer953 that's gonna be painful to type 😅 but totally understand the desire.

@tomer953
Copy link
Contributor

@tomer953 that's gonna be painful to type 😅 but totally understand the desire.

besides that,
it would diverge from the conventional usage of RxJS, where functions like from expect a single argument in the form of an array

@renatoaraujoc
Copy link
Author

renatoaraujoc commented Sep 20, 2023

Hey, the first feedback makes sense to me but I don't quite get the 2nd feedback. Can you elaborate a bit?

Hey Chau, yea I can elaborate.

The beauty of working with computedFrom is that it also accepts anything and you dont have to do miraculous stuff to get it going, it accepts Observables and Signals, right?

Use my last example:

function filterNotNullOrUndefined<T>(
    input: null | undefined | T
): input is T {
    return input !== null && input !== undefined;
}

// I use this function alot in my codebase
function rxjsFilterNotNullOrUndefined<T>() {
    return (source$: Observable<null | undefined | T>) =>
        source$.pipe(filter(filterNotNullOrUndefined));
}

// Desirable
computedNumberToSomething = computedFrom<number>(
    this.someNumber, // imagine this is <number | null | undefined>
    pipe(
        // now I want to filter the typeguard function above, this is now NonNullable<number>
        rxjsFilterNotNullOrUndefined(),
        map((number) => someNumber + 1)
    )
);

// Current
computedNumberToSomething = computedFrom<number>(
    [this.someNumber], // imagine this is <number | null | undefined>    
    pipe(
        // typeguard won't infer the 1st param is NonNullable<number>
        filter(([number]) => !!number),
        // Error: number can be undefined or null
        map(([number]) => someNumber + 1 )
     ) 
);
  1. I'm having to forcedly cast my computedFrom to the result value like: computed = computedFrom<[number], number]> or my type inference will show me unknown on my templates.

  2. I also had issues working with Prisma.ModelGetPayload<typeof modelArgs> for one-to-many relationships, my code will returning an error when accessing computed.oneToManyModel.someValue, it says called someValue on an undefined value error when it's not undefined at all. I can elaborate on this more later.

Unless you have a better way to handle these situations, I'm all ears :)

Renato

@tomer953
Copy link
Contributor

Unless you have a better way to handle these situations, I'm all ears :)

The problem you mentioned is not related to this library but its an old and famous problem in typescript

image

@tomer953
Copy link
Contributor

B.t.w:

I use this function alot in my codebase

function filterNotNullOrUndefined<T>(
    input: null | undefined | T
): input is T {
    return input !== null && input !== undefined;
}

// I use this function alot in my codebase
function rxjsFilterNotNullOrUndefined<T>() {
    return (source$: Observable<null | undefined | T>) =>
        source$.pipe(filter(filterNotNullOrUndefined));
}

Check out the brand new filterNil operator
https://ngxtension.netlify.app/utilities/filter-nil/

@renatoaraujoc
Copy link
Author

Unless you have a better way to handle these situations, I'm all ears :)

The problem you mentioned is not related to this library but its an old and famous problem in typescript

image

Yea, I know this issue, but we can have some workarounds to mirigate this limitation, my suggestion was targeted for this.

@tomer953
Copy link
Contributor

tomer953 commented Sep 21, 2023

Oh I see.
I not so sure it’s related to the swapping of the generics, but i’m sure @nartc will help here :)

I think its better to separate to a new issue
#76
And if its related just close them both

@nartc
Copy link
Collaborator

nartc commented Sep 21, 2023

I'll take a closer look this weekend.

@nartc
Copy link
Collaborator

nartc commented Oct 13, 2023

I now have time to take a look at this.

  1. What happens if I don't want to use any operator with computedFrom? I simply want to combine some Signals and Observables together. Specifying the Output first won't allow me to default it to the type of the Input

// Desirable
computedNumberToSomething = computedFrom<number>(
    this.someNumber, // imagine this is <number | null | undefined>
    pipe(
        // now I want to filter the typeguard function above, this is now NonNullable<number>
        rxjsFilterNotNullOrUndefined(),
        map((number) => someNumber + 1)
    )
);

Your desirable case won't work because what happens if:

// Desirable
computedNumberToSomething = computedFrom<number>(
    this.someNumber, // imagine this is <number | null | undefined>
    this.someOtherValue,
    pipe(
        // now I want to filter the typeguard function above, this is now NonNullable<number>
        rxjsFilterNotNullOrUndefined(), // now what happens?
        map((number) => someNumber + 1)
    )
);

Regardless of how we want computedFrom to accept (array, dictionary, or a rest arguments), the pipe() would still need to expect a collection of values of all the inputs. Take a look at this scenario

computedFrom(
    [of<number | null>(1), signal(2)],
    pipe(
        filter(([first]) => !!first), // here we're saying if first is falsy, then stream won't emit.
        map(([first, second]) => {}) 
    )
);

// in this case, I'd recommend
computedFrom(
    [of<number | null>(1).pipe(filterNil()), signal(2)],
    pipe(
        map(([first, second]) => {})
    )
)

@eneajaho
Copy link
Collaborator

@renatoaraujoc looks like this is not doable. Is it ok if we close this?

@renatoaraujoc
Copy link
Author

Hey @eneajaho, it should be doable on next versions of TS but today it's definitely not. Let's close it :)

@eneajaho eneajaho closed this as not planned Won't fix, can't repro, duplicate, stale Nov 15, 2023
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

No branches or pull requests

4 participants