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

Incomplete TypeScript typings in @graphiql/toolkit@0.3.0 #1989

Closed
DanielSWolf opened this issue Oct 28, 2021 · 7 comments · Fixed by #1990, #1992 or #1993
Closed

Incomplete TypeScript typings in @graphiql/toolkit@0.3.0 #1989

DanielSWolf opened this issue Oct 28, 2021 · 7 comments · Fixed by #1990, #1992 or #1993
Labels

Comments

@DanielSWolf
Copy link

DanielSWolf commented Oct 28, 2021

In version 0.3.0, @graphiql/toolkit dropped the dependency on subscriptions-transport-ws, making it a devDependency only. The problem is that @graphiql/toolkit/dist/create-fetcher/types.d.ts still performs a type import from subscriptions-transport-ws. This means that any TypeScript project using @graphiql/toolkit (or graphiql) but not subscriptions-transport-ws will now get a TypeScript error:

node_modules/@graphiql/toolkit/dist/create-fetcher/types.d.ts:3:41 - error TS2307: Cannot find module 'subscriptions-transport-ws' or its corresponding type declarations.

@acao
Copy link
Member

acao commented Oct 28, 2021

Oof! My bad, I can patch that today.

@acao acao added the bug label Oct 28, 2021
@acao
Copy link
Member

acao commented Oct 28, 2021

@ardatan what do you think we should do here? Just replace it with an unknown type? The legacy client users would lose some type fidelity… trying to remember if there is any trick for inlining types from elsewhere

@DanielSWolf
Copy link
Author

Oof! My bad, I can patch that today.

That would be awesome! (Our CI is currently standing still.)

@DanielSWolf
Copy link
Author

Regarding the SubscriptionClient type: Substituting unknown would make the typings work for producers of that type (anything can be assigned to unknown), but not for consumers (you can't do much with an unknown).

I don't know enough about this package to know whether that would suffice. If not, any might be a safer choice.

@DanielSWolf
Copy link
Author

@acao I'm afraid the new version 0.3.1 of @graphiql/toolkit still has the same problem.

The new typings file @graphiql/toolkit/dist/create-fetcher/types.d.ts looks like this:

// ...
import type { SubscriptionClient } from 'subscriptions-transport-ws'; // 1
//...
export declare type WebsocketsClient = Client | SubscriptionClient; // 2
export interface CreateFetcherOptions {
    url: string;
    subscriptionUrl?: string;
    wsClient?: Client;
    legacyClient?: SubscriptionClient; // 3
    headers?: Record<string, string>;
    wsConnectionParams?: ClientOptions['connectionParams'];
    enableIncrementalDelivery?: boolean;
    fetch?: typeof fetch;
    schemaFetcher?: Fetcher;
}

So the import of SubscriptionClient from subscriptions-transport-ws still occurs (1) and is used in two places: the definition of WebsocketsClient (2) and the definition of CreateFetcherOptions (3).

Could you make another attempt, ideally checking the generated types file to make sure the import is indeed gone?

@acao
Copy link
Member

acao commented Oct 29, 2021

@DanielSWolf thanks for reporting that! i have a follow up PR that finalizes this, and also re-introduced a step to github actions that validates the imported build in a simple webpack project

@DanielSWolf
Copy link
Author

Thank you, @acao! It took us a few hours to verify the fix due to a problem with our local NPM proxy, but now everything is working fine again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants