Potentially unintentionally large AoE breaking changing to typescript `MockedResponse` in type 3.9

[destin-estrela]

Issue Description

Overview

The MockedResponse had breaking changes to its types in 3.9 to allow the ability to dynamically match mocks.

- export type ResultFunction<T> = () => T;
+ export type ResultFunction<T, V = Record<string, any>> = (variables: V) => T;

export interface MockedResponse<
  TData = Record<string, any>,
  TVariables = Record<string, any>,
  {
  request: GraphQLRequest<TVariables>;
-  result?: FetchResult<TData> | ResultFunction<FetchResult<TData>>;
+  result?: FetchResult<TData> | ResultFunction<FetchResult<TData>, TVariables>;

But it came with a potentially unintended side affect. Here I'd like to get the contributors thoughts on this problem and suggest a solution.

The breaking change

This change affected the usability of the MockedResponse type. Constructing a strongly typed MockedResponse is useful for constructing mocked responses with type hints from the editor before supplying it into the mock link. However, you can no longer declare a MockedReponse of a specific type:

const response: MockedResponse<MyDocument, MyVariables> = {...}

And pass it into a function that receives a generic list of MockedResponse

function createMockLink(mocks: MockedResponse[]) {
  const mockLink = new MockLink(mocks, true);
  // ...
}

createMockLink([response])

This will return an error that didn't use to happen in 3.8:

Type 'ResultFunction<MyDocument, MyVariables>' is not assignable to type 'ResultFunction<Record<string, any>, Record<string, any>>'.

Type 'Record<string, any>' is not assignable to type 'MyDocument'.

Even though the MockedResponse is internally type-safe, typescript is unable to infer this when passing it into a generic list of MockedResponse. It doesn't know that we are only calling the function with the variables that we give it, which were type-safe upon construction.

You can see a simplified reproduction of this typescript limitation here.

The current official migration

The official typing of the MockLink got around this in an unwieldy way. The constructor used to simply take a list of MockedResponse

export declare class MockLink extends ApolloLink {
    // ...
    constructor(mockedResponses: ReadonlyArray<MockedResponse>, // ...
}

But now it uses any for the type arguments to avoid this issue.

//@apollo/client/testing/core/mocking/mockLink.d.ts:23
export declare class MockLink extends ApolloLink {
    // ...
    constructor(mockedResponses: ReadonlyArray<MockedResponse<any, any>> //...
}

The consequences

This behavior has leaked into the public interface. API consumers must now explicitly define MockedResponse<any, any>[] any time they are declaring a generic list of MockedResponse that receive more highly specified MockedResponse. Depending on the repository style guide, linter errors will need to be suppressed, or a wrapper type defined to avoid using any explicitly. In addition, the typescript error is confusing. Users may wonder why their more specific type doesn't get cast automatically to the looser Record<string, any> type of the default MockedResponse (this was okay until the generic was used inside function arguments).

In the spirit of encouraging type-safe code and avoiding the burden on developers to come to this conclusion on their own, I suggest the following solution.

A potential solution

To fix this this without sacrificing type safety consider changing the interface of MockedResponse from:

export interface MockedResponse<TData = Record<string, any>, TVariables = Record<string, any>> 

to

interface MockedResponse<TData extends Record<string, any> = any, TVariables extends Record<string, any> = any>

In most cases, when users construct their own MockedResponse, typescript will infer the correct type from the usage of TData and validate that it adheres to Record<string, any>.

interface MockedResponse<TData extends Record<string, any> = any, TVariables extends Record<string, any> = any> {
    // providing these fields allows typescript to narrow the type
    // implicitly, and it will still check that its Record<string, any>
    request: GraphQLRequest<TVariables>;
    result?: FetchResult<TData> | ResultFunction<FetchResult<TData>, TVariables>;
    variableMatcher?: VariableMatcher<TVariables>;
    newData?: ResultFunction<FetchResult<TData>, TVariables>;
}

While still preserving the ability to pass in responses into a MockedResponse[] without the need to decode the error and resort to specifying MockedResponse<any, any>[]

Conclusion

3.9 introduced breaking API changes that subtly affect the usability of working with MockedResponse[]. I believe we can restore the old flexibility while still supporting the new features. I'd like to hear what the maintainers think about this issue and the potential solution, or if this is intentional and here to stay. Thank you for your feedback.

Link to Reproduction

https://www.typescriptlang.org/play/?#code/ATAuE8AcFNgJWgZwK4BtQDFkDsDGoBLAe2wB4BBAGjgD5gBeYACgEMAnAcwC5hyBKBnTgBuAFCiQEGMACyRXAGtoAEwSJIJRNAoN40XETbLSiUGwLYOlYC2zga1uLoQGjJsxau37dRgG8JEBtOHnIxIOAAMxxcHjU0TBjCEgpqGnDgAF9xSShYGXAAEXlkAFtobFBdAIiWHmwygCNoNkDswKl88AA1dgIWRtQkasCQRvqmlracqKTibGBcFlRULDxEJmi8AHE2ImRIRB45RRU1DWwtAG0AXQEaoK3cXf3DgDpIwwBRFlwAC02MReB0Esx2ewOHxigPBrze7A4fD400CBkuVTYSAuWmO8iUqixmm0BWKuDKFVA1gKvXMAyGiF8wAeICePARoKYfnGwQ48MyfEoox5PD8dWAACZsiB2iAAPSy4DQABuFTAf32HD+wAA7rAFNgiNq1QREMATWA8sBECxIrBEBZcLAlis1rhEIF5WbTcttSxwKbnahPGrYE9kgttQRQFqo6bDQtIJjlNBIhYVDzyZVBXKFZ1ELhzJAqktsAByKoWW1sNUsUBvVHLVYxDZXTHqIl3YRAA

Reproduction Steps

No response

@apollo/client version

3.10.3

[phryneas]

Yeah, unfortunately TVariables is used in a contravariant position here, which forces the variance of the generic argument to in out.

Generally: while we try to guarantee "no breaking changes" for runtime code, that is not true for TypeScript annotations - sometimes we just have to break a type's behaviour to enable an improvement in developer experience in the long term. But of course, we try to keep these changes to a minimum.

The change here is hard to avoid, and using any as a generic argument default is a solution we really want to try to avoid. any is a great tool when used deliberately (and while it should be used sparingly, you probably shouldn't lint against it), but it should be deliberately used in userland code, not as a default.

I'm investigating a few unorthodox solutions for this (see this tweet, but I can't guarantee that anyone will come out of it.

In the meantime, can I suggest that you use a helper type like

type MockedResponseArray = ReadonlyArray<MockedResponse<Record<string, any>, any>>

?

That way you introduce any only in one place in your codebase.

[phryneas]

Could you please try #11848 and report back if that works for you?

npm i @apollo/client@0.0.0-pr-11848-20240516124842

[destin-estrela]

Could you please try #11848 and report back if that works for you?

npm i @apollo/client@0.0.0-pr-11848-20240516124842

@phryneas

This appears to work!

[github-actions[bot]]

Do you have any feedback for the maintainers? Please tell us by taking a one-minute survey. Your responses will help us understand Apollo Client usage and allow us to serve you better.

[github-actions[bot]]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. For general questions, we recommend using StackOverflow or our discord server.