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

Changes to Variables introduces type unsafety/churn #472

Closed
srittau opened this issue Mar 6, 2023 · 3 comments · Fixed by #659
Closed

Changes to Variables introduces type unsafety/churn #472

srittau opened this issue Mar 6, 2023 · 3 comments · Fixed by #659
Labels

Comments

@srittau
Copy link

srittau commented Mar 6, 2023

Description

Please consider the following code, which worked fine with graphql-request 5.1.0:

import { Variables, request } from "graphql-request";

interface Vars {
    bar: string;
}

void request<unknown, Vars>("...", "...", {
    bar: "baz",
});

If unknown variables are used, typescript would fail to compile:

// ...

void request<unknown, Vars>("...", "...", {
    bar: "baz",
    wrong: 123, // <-- compile error
});

With graphql-request 5.2.0, Vars needs to extend from Variables, otherwise typescript complains that "Index signature for type 'string' is missing in type 'Vars'". But extending from Variables (and therefore introducing an index signature) means that unknown variables are not flagged anymore:

import { Variables, request } from "graphql-request";

interface Vars extends Variables {
  bar: string;
}

void request<unknown, Vars>("...", "...", {
  bar: "baz",
  wrong: 123, // <-- not flagged
});

I wasn't unable to identify the exact change that caused it, unfortunately.

@srittau
Copy link
Author

srittau commented Mar 20, 2023

And just to leave this here: The workaround/fix is to change

interface Vars {
    bar: string;
}

to

type Vars = {
    bar: string;
}

The problem is that interface Vars is extensible, so typescript doesn't believe it's compatible to Record<...>, while type Vars is not.

@jasonkuhrt jasonkuhrt pinned this issue Apr 5, 2023
@jasonkuhrt
Copy link
Member

jasonkuhrt commented Apr 5, 2023

Thanks @srittau for the issue. Adding a test case and fix for this is on my radar.

@jasonkuhrt
Copy link
Member

I haver a PR close to solving this, thanks again.

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

Successfully merging a pull request may close this issue.

2 participants