urql-graphql / urql

The highly customizable and versatile GraphQL client with which you add on features like normalized caching as you grow.
https://urql.dev/goto/docs
MIT License
8.64k stars 452 forks source link

(svelte) - Generic on `subscription`'s store not inferring for altered results #1716

Closed V-ed closed 3 years ago

V-ed commented 3 years ago

When using the subscription handler as described by the official docs, TypeScript spews errors related to types :

image

Looking at the types, the subscription's handler uses the generic Result type : image

and the SubscriptionHandler's type does not mention an array type either as the prev parameter : image

This makes it particularly annoying to use in TypeScript as you need to ignore those type errors, even though the code works as expected.


urql version & exchanges:

import { SubscriptionClient } from 'subscriptions-transport-ws';
const subscriptionClient = new SubscriptionClient(`ws://${apiUrl.host}/graphql`, { reconnect: true });

exchanges: [
    ...defaultExchanges,
    subscriptionExchange({
        forwardSubscription(operation) {
            return subscriptionClient.request(operation);
        },
    }),
],

Steps to reproduce

  1. Use urql/svelte with any (probably strict) typescript setup
  2. Implement the basic subscription example from the official docs
  3. See type errors

Expected behavior

No type errors, prev parameter typed as array of R (R[]) or Result being of array type in the subscription method type def : image

Therefore getting no compile errors nor runtime errors.

Actual behavior

Non array type gives typescript compile error, but no runtime error (latter being expected).

kitten commented 3 years ago

Sorry, but I think that looks like an expected type inference from TypeScript here to me 😅

When you default prevMessages to an empty array without specifying a type then TypeScript will automatically instantiate it as never[] as an exact type. Hence this may be resolved by specifying an explicit return type, or better, by annotating the prev argument with the accumulated / returned type.

V-ed commented 3 years ago

I could understand that never[] might be an issue, but what about this?

image

Seems weird to me that it is inferred as any[]... I'm guessing because of the return type, which is therefore inferred as any[]?

How would you tackle this then? I've tried many different ways, while still following the official docs, but none seems to fix this typing issue...

You've mentionned

by annotating the prev argument with the accumulated / returned type.

But I'm not too sure what you mean by that

V-ed commented 3 years ago

I understand that this error happens because the generics are setup like so, but I think it may not be right when used with TypedDocumentNode (https://github.com/dotansimha/graphql-typed-document-node) :

image

(it says DocumentNode here because my codegen somehow imports TypedDocumentNode as DocumentNode, as shown below)

import { TypedDocumentNode as DocumentNode } from '@graphql-typed-document-node/core';

Therefore, the types are as follows :

This is why the above does not show any errors anymore, but while no errors are shown, there are many issues with this (when the store's query is a TypedDocumentNode) :

  1. data should not be any, it should be of the Result type (in this case, NewMessageSubscription);
  2. prevMessage should not be of type Result, but rather of the return value of the handler function itself
    • in other words, handler?: SubscriptionHandler<Data, Result> could be handler?: SubscriptionHandler<Data>

This makes it a bit annoying to deal with in Typescript as types could be inferred but are instead all over the place. Why do I need to explicitely define each parameters types, and even break because if my store does not respect the handler's types, typescript still isn't happy? (see below)

image

kitten commented 3 years ago

Ok, I believe there may be a misunderstanding here, because the type cannot be inferred. This is explained in the docs:

It receives the previous set of data that this function has returned or undefined. As the second argument, it receives the event that has come in from the subscription. You can use this to accumulate the data over time, which is useful for a list for example.

What this means is, the handler is like a reducer function passed to Array.prototype.reduce. It can produce any return value, which is what you're doing here. So what happens is that prev and the return value are a different generic than the result.

TypeScript now has two places where it could infer the value from: the return value or the prev argument. The prev argument actually is the generic and defaults to Data, the query's type, since that's what it does when no handler is passed; it returns the subscription's event data unaltered.

Now, when you do prev = [], TypeScript actually uses that as type information. However, because prev is defaulted to an empty and typeless array literal, it becomes never[]. Assigning the return type also doesn't provide clarity to TS, since when you use this literal, its type isn't inferable.

Here's a TS Playground Example of what's going on.

Once we basically do this, TypeScript knows your intention:

subscription(exampleQuery, (prev: number[] = [], value) => {
  return [...prev, value.example];
});

In other words, we cannot give TypeScript no type information when we're already giving it conflicting type information. In your last screenshot there's an issue as well in which prevMessages isn't accounting for undefined anymore. Also, you don't have to provide all type annotations. The data argument will in fact continue to be inferred.

Edit: I'll close this for now, since I think it's purely a usage issue for now :v: but feel free to keep responding or asking questions about this!

V-ed commented 3 years ago

In your TS example, I see that your value parameter is inferred as Data, but mine is still inferred with any, and I get this :

image

As you can see, NewMessagesSubscription (the type given by the TypedDocumentNode, here being the variable NewMessagesDocument) is clearly not an array and therefore will never match the prevMessages parameter if the latter is an array...

I really am not sure what is going on here; I understand your point, but I am unable to make it work in my setup. I know it's not the most barebone repro, but can you take a look in here to see what am I doing wrong? https://github.com/V-ed/svelte-typescript-routing-template/tree/5d233206e4e363b6767e9ac8bb0c79e834be54af

The file in question is src/pages/chat/index.svelte

I can understand closing the issue since it might indeed be usage error, maybe we can even transfer all this into a Discussion tab, but ah well, we'll see how this goes.

V-ed commented 3 years ago

I also tried tweaking your TS playground to match more closely what the types of urql gives, and while it still doesn't throw errors (unlike in my code), the value parameter is now inferred as any, not Data (or DataTest in my playground) anymore...

Edit : missed link to playground : TS Playground

V-ed commented 3 years ago

Tweaked my own TS Playground a bit simply by changing the query parameter of the subscription method from OperationStore<Result> to OperationStore<Data> and immediately, the value parameter is of the right type, and not of any :

Updated TS Playground

I think this clearly shows that something is wrong; I might do something wrong (even though I can't figure it out yet), but it seems clear to me that using Result for the store parameter was wrong :

image

Don't hesitate saying that I'm wrong, I just want to understand what am I missing with the best of my knowledge :)

Thanks btw for the follow ups @kitten !

V-ed commented 3 years ago

EDIT

I have no idea what I changed on my side but I do not have that issue anymore indeed, so I think 1.3.0 fixed it for me, sorry if you still had confusion about this! Thanks for the fix/tweak!


~Am I doing something wrong? This is still happening in 1.3.0 :~

image

~Do I really need to do this (specifying types in generic definition) for every single subscription actions that I will do?~

image


~In the mean time, I'll keep using my own definition as I find it much easier to write my subscriptions without specifying the operationStore type.~

~As you said here though, returning the Result is wrong, so I changed it to this :~

import { OperationStore, SubscriptionHandler } from '@urql/svelte';

declare module '@urql/svelte' {
    export declare function subscription<Data = any, Result = Data, Variables = object>(
        store: OperationStore<Data, Variables>,
        handler?: SubscriptionHandler<Data, Result>,
    ): OperationStore<Data, Result, Variables>;
}

~Just a general note, I'd like to see a mention in the docs that mention something along the lines of "to prevent typescript compiling errors and get proper typing, you may need to explicitly define the accumulator to your data type", as this would help a lot typescript folks using svelte's stores. Just my 2 cents on this :)~

~Sorry that I keep saying that this isn't fixed, I just want to do it the proper way and I'm not sure the "proper way" has been settled yet...~