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.68k stars 452 forks source link

Suggestion: Export withPromise for easier typing in typescript tests #3133

Open probablykabari opened 1 year ago

probablykabari commented 1 year ago

https://github.com/urql-graphql/urql/blob/9cdb74b03e07d46e056ef023d1543f24a823ec55/packages/core/src/utils/streamUtils.ts

If you're coming from v3 -> v4 in typescript the following results in a type error because this is a Source<OperationResult> instead of OperationResultSource<OperationResult>:

   // on a mock Urql client
    executeQuery: () => {
       const result = makeResult(....) // OperationResult
        return fromValue(result) // Source<OperationResult>
    };

It's a small thing but I think it is better to have access to the util than use as OperationResultSource<OperationResult> for anyone consuming the library.

kitten commented 1 year ago

Personally, I'm a little torn on this.

Of course, turning a Source<T> into an OperationResultSource<T> is “trivial” and withPromise could be exported, there's something to be said about:

Rather, it can actually, I believe, lead to uncaught bugs in tests. The reason being that it encourages testing a single case one by one.

So, I think as OperationResultSource<T> is preferable for the time being because it enforces the "conscious" decision to mock the Client.

However, this kind of mocking isn't perfect and doesn't handle:

In short, I'd much rather bite the bullet and think about mocking utilities that emulate the full Client more closely. The reason being that the Client has become more burdened with control-flow / event hub logic, and that a mock with a cast is imho 100% fine for the purpose of mocking the Client for our bindings.

So, instead, I think it's time to maybe think about @urql/testing and to add some test utilities, which allow for:

How does that sound? I think this gets us further than exposing withPromise, which I think would just be a crutch

probablykabari commented 1 year ago

I spoke too soon, I'm in the midst of upgrading and withPromise definitely doesn't work with subscriptions (as you implied).

+additional side note, using as OperationResultSource<T> makes it impossible to test anything that uses the client directly.

I agree with the testing library. Having used Urql since 1.x I have bits and pieces of test utilities all over the place in different projects and having a more first-class library would be a great addition. I like the idea of doing it via an exchange, we do something similar to test relay pagination. Happy to take a stab at making an initial PR for that.