graffle-js / graffle

Simple GraphQL Client for JavaScript. Minimal. Extensible. Type Safe. Runs everywhere.
http://graffle.js.org/
MIT License
5.87k stars 308 forks source link

[5.2.0] broke the typings of graphql-codegen generation #467

Closed efstathiosntonas closed 1 year ago

efstathiosntonas commented 1 year ago

Description

On 5.2.0 the dist folder has been removed in favor of cjs/esm. I do understand that graphql-codegen should update their typings generation flow to match 5.2.0, just wanted to bring this into your attention.

error thrown: TS2307: Cannot find module 'graphql-request/dist/types.dom' or its corresponding type declarations.

and the import is: import * as Dom from 'graphql-request/dist/types.dom';

ps. I'm using:

    "@graphql-codegen/cli": "^3.2.1",
    "@graphql-codegen/typescript-graphql-request": "^4.5.8"
gusfune commented 1 year ago

Can confirm same issue. Rolling back to 5.1.0 fixed temporarily.

efstathiosntonas commented 1 year ago

Issue is located here: https://github.com/dotansimha/graphql-code-generator-community/blob/890aabe2c018a39e358dca5896b2b2fd9148fd4e/packages/plugins/typescript/graphql-request/src/visitor.ts#L51-L52

jmoore240 commented 1 year ago

Can also confirm this issue

batmanhit commented 1 year ago

This issue is happening as well.

jasonkuhrt commented 1 year ago

Wow had no idea internals were being depended upon like this. This is what exports map is for, explicit contracts around entry points that is actually maintainable by library authors.

The new exports also do not expose the types.dom entry point so graphql-codegen cannot be updated to work currently.

I'll need to understand what the use-case was and how to properly support it officially going forward. Any help in doing that (understanding) appreciated. I'll try to investigate later this month myself either way.

SimonSchneider commented 1 year ago

I'll need to understand what the use-case was and how to properly support it officially going forward. Any help in doing that (understanding) appreciated

This might not be the only use-case, but what is being used by our generated code is the Dom.RequestInit["headers"] as shown in the example below:

import { GraphQLClient } from 'graphql-request';
import * as Dom from 'graphql-request/dist/types.dom';

export function getSdk(client: GraphQLClient, withWrapper: SdkFunctionWrapper = defaultWrapper) {
  return {
    XYZ(variables: XYZMutationVariables, requestHeaders?: Dom.RequestInit["headers"]): Promise<XYZMutation> {
      return withWrapper((wrappedRequestHeaders) => client.request<XYZMutation>(XYZDocument, variables, {...requestHeaders, ...wrappedRequestHeaders}), 'XYZ', 'mutation');
    },
  };
};

basically it generates named wrapper functions around client.request(document, vars, headers) for each Query/Mutation. When doing this it needs access to the variables and headers types. And I'm guessing the headers types where easiest to get from the Dom type.

Exporting the header type from this library and changing the codegen library here seems like it should be enough: link

    this._additionalImports.push(`${typeImport} { GraphQLClient } from 'graphql-request';`);
    this._additionalImports.push(
      `${typeImport} * as Dom from 'graphql-request/dist/types.dom${fileExtension}';`,
    );

I'm guessing if the headers are exported next to the grapqlClient the import could be merged with the line above.

And the references to the headers (Dom.RequestInit["headers"]) could be updated in the lines below: link

          }, requestHeaders?: Dom.RequestInit["headers"]): Promise<{ data: ${
            o.operationResultType
          }; extensions?: ${this.config.extensionsType}; headers: Dom.Headers; status: number; }> {
    return withWrapper((wrappedRequestHeaders) => client.rawRequest<${
      o.operationResultType
    }>(${docArg}, variables, {...requestHeaders, ...wrappedRequestHeaders}), '${operationName}', '${operationType}');
}`;
        }
        return `${operationName}(variables${optionalVariables ? '?' : ''}: ${
          o.operationVariablesTypes
        }, requestHeaders?: Dom.RequestInit["headers"]): Promise<${o.operationResultType}> {
  return withWrapper((wrappedRequestHeaders) => client.request<${
    o.operationResultType
  }>(${docVarName}, variables, {...requestHeaders, ...wrappedRequestHeaders}), '${operationName}', '${operationType}');
}`;
efstathiosntonas commented 1 year ago

@jasonkuhrt can you please pin this issue for visibility? Thanks

kevindice commented 1 year ago

Wow had no idea internals were being depended upon like this. This is what exports map is for, explicit contracts around entry points that is actually maintainable by library authors.

Hyrum’s Law—“With a sufficient number of users, every observable behavior of your system will be depended upon by someone.”

:)

Appreciate your efforts on maintaining this.

justb3a commented 1 year ago

There is a workaround using hooks to fix the file after generation. Found the idea here: https://github.com/dotansimha/graphql-code-generator-community/issues/501, see also Lifecycle Hooks.

For 5.2.0 it's fine to replace graphql-request/dist/types.dom with graphql-request/src/types.dom using:

hooks:
  afterOneFileWrite:
  - 'gsed -i -e"s|graphql-request/dist/types\.dom|graphql-request/src/types.dom|g"'

For 6.0.0 For me, my code generation uses only RequestInit from the "Dom" named import statement. RequestInit is available by default in the typescript package. So I adapted the hook, removed the import statement and replaced all Dom.RequestInit with RequestInit. Does work for now.

hooks:
  afterOneFileWrite:
    - 'gsed -i -e"s|Dom\.RequestInit|RequestInit|g"'
    - 'gsed -i -e"/graphql-request\/dist\/types\.dom/d"'

Edit: Update still not possible since at least for me since @graphql-codegen/typescript-graphql-request requires ~5.1.0 as a peer dependency 😕

efstathiosntonas commented 1 year ago

fixed on @graphql-codegen/typescript-graphql-request@5.0.0 https://github.com/dotansimha/graphql-code-generator-community/releases/tag/release-1684417928535

Zerebokep commented 1 year ago

Thank you so much @@michaelgmcd, you rock. Most problems are gone now, however, I still have this one:

Argument of type 'HeadersInit | undefined' is not assignable to parameter of type 'RequestInit | undefined'.

Screenshot
efstathiosntonas commented 1 year ago

@Zerebokep let me know if you wish to reopen this

Zerebokep commented 1 year ago

@efstathiosntonas Yes, please. I think RequestInit must be replaced with HeadersInit. I haven't checked it yet, but it probably got renamed in graphql-request 6.1

Anthony-Jhoiro commented 1 year ago

Hello @efstathiosntonas, I noticed an issue in this release (I can create a new issue if needed but it is related to this one)

The change int he import for "'graphql-request/build/cjs/types';" was not applied everywhere, and the Dom.Headers occurrence create typing errors. A simple way to see the errors is just by using this configuration with at least one query

const config: CodegenConfig = {
    schema: `${graphqlServer}/graphql`,
    documents: ['src/**/*.gql'],
    generates: {
        './src/server/generated/requests.ts': {
            plugins: ['typescript', 'typescript-operations', 'typescript-graphql-request'],
            config: {
                rawRequest: true
            },
        }
    }
}
export default config
madebyfabian commented 1 year ago

What's the current state of this? I am on 5.0.0 and the generated code does not include any import for the Dom interface. Therefore, I get:

⚠ Error(TS2503)  | 
Cannot find namespace 'Dom'.

My config:

import type { CodegenConfig } from '@graphql-codegen/cli'
import { loadEnv } from 'vite'

const { CAISY_PROJECT_ID, CAISY_API_KEY } = loadEnv(process.env.NODE_ENV as string, process.cwd(), '')

const config: CodegenConfig = {
    overwrite: true,
    schema: {
        [`https://cloud.caisy.io/api/v3/e/${CAISY_PROJECT_ID}/graphql`]: {
            headers: {
                'x-caisy-apikey': CAISY_API_KEY || '',
            },
        },
    },
    generates: {
        'gen/caisy.gen.ts': {
            documents: 'src/gql/**/*.gql',
            plugins: ['typescript', 'typescript-operations', 'typescript-graphql-request'],
            config: {
                rawRequest: true,
                useTypeImports: true,
            },
        },
    },
}
export default config
fungilation commented 1 year ago

before you ask a question like this, try the latest version first.

I'm on 6.1.0 and graphql-codegen runs fine with it

madebyfabian commented 1 year ago

@fungilation I of course mean the package version of @graphql-codegen/typescript-graphql-request. Which is actually the most recent at 5.0.0. I am also on the most recent version of graphql-request (6.1.0).

RobbyUitbeijerse commented 1 year ago

hey @madebyfabian , I was in the same situation and it indeed is resolved in @graphql-codegen/typescript-graphql-request - but it's not included in a stable release yet - 5.0.0 indeed still contains the Dom.Headers type usage.

You can find that Dom.Headers was changed to just Headers by looking in the plug-in source here: https://github.com/dotansimha/graphql-code-generator-community/blob/main/packages/plugins/typescript/graphql-request/src/visitor.ts#L132C55-L132C56

But you need to install the v6 alpha to actually get it, if you install @graphql-codegen/typescript-graphql-request@6.0.0-alpha-20230811215938-77de575f5 and you should be good to go. The alpha seems stable enough (at least for me).

Alternatively you could do something similar to https://github.com/jasonkuhrt/graphql-request/issues/467#issuecomment-1531058012 , and simply replace Dom.Headers for Headers after generating the client.

Hope that helps.

madebyfabian commented 1 year ago

@RobbyUitbeijerse Thank you very much for the detailed update! I upgraded the package to the v6 alpha and don't have any type errors anymore.

RobSchilderr commented 1 year ago

@graphql-codegen/typescript-graphql-request@6.0.0-alpha-20230811215938-77de575f5

This also solved it for me!

Zerebokep commented 1 year ago

@efstathiosntonas Yes, please. I think RequestInit must be replaced with HeadersInit. I haven't checked it yet, but it probably got renamed in graphql-request 6.1

I realised this error was related to my custom fetcher, everything is fine now. Thank you again.