cometkim / gatsby-plugin-typegen

Let's give developers using GatsbyJS better DX with extreme type-safety
https://www.gatsbyjs.org/packages/gatsby-plugin-typegen/
MIT License
204 stars 23 forks source link

Multiple definitions for query #44

Closed Js-Brecht closed 4 years ago

Js-Brecht commented 4 years ago

Do you mind taking a look at this starter? I am getting the error show below in vscode-apollo, and I'm not sure if I should start here, or with vscode-apollo. It almost seems like it could be happening because the query exists in the image component, as well as in plugin-documents.graphql.

image

I've noticed this same error in other repositories, and from time to time it will make the plugin crash. Disabling one or the other of

// apollo.config.js
        includes: [
            `${__dirname}/src/**/*.{ts,tsx}`,
            `${__dirname}/.gatsby/graphql/plugin-documents.graphql`,
        ], // eslint-disable-line graphql/template-strings

will fix the error, but then I don't get full support from the plugin.

If this is not an issue caused by a conflict between gatsby-plugin-typegen and vscode-apollo, or a misconfiguration, but is instead an issue solely with vscode-apollo, then I will open an issue there.

cometkim commented 4 years ago

Hi @Js-Brecht, I can't reproduce this on your starter...

image

Any more particular settings you made ahead of the starter?

Js-Brecht commented 4 years ago

No special configuration. Just what you see in the repository. I'm on a different computer right now, so I had to download it and run it fresh. I had to do a build for it to generate the schema with the duplicate query definition, then reloaded the window to restart vscode-apollo. As soon as vscode finished reloading, I get this: image and this: image

Js-Brecht commented 4 years ago

If I comment out <Image /> in pages/index.tsx and also comment out the static query in components/image.tsx, I will then get errors for the Seo query in components/seo.tsx. But again, I have to do a build to generate the schema, and reload vscode (or could probably just do Apollo: Reload Schema) before it generates the error.

cometkim commented 4 years ago

So this problem because of the documents are emitted even they are contained in src

That... should be filtered out from here:

https://github.com/cometkim/gatsby-plugin-typegen/blob/3457db9dfef41ca0eba5c728adef3a61e7979f7a/src/gatsby-node.ts#L117

Can you try again on the latest version? (v1.1.1)

cometkim commented 4 years ago

I just realized that I have to deal with outside of src to support gatsby-node. Thank you so much @Js-Brecht !

Js-Brecht commented 4 years ago

Can you try again on the latest version? (v1.1.1)

Same problem.

I think I have found one issue:

  console.log(path.resolve(basePath, 'src'));
  console.log(trackedSource.keys());
  process.exit(0);

  const pluginDocuments = Object.values(emitPluginDocuments).some(Boolean) && (
    stripIndent(
      Array.from(trackedSource.entries())
        .filter(([componentPath]) => !componentPath.startsWith(path.resolve(basePath, 'src')))
        .map(([, source]) => source.rawSDL)
        .join('\n'),
    )
  );

will return

D:\dev\source\gatsby\gatsby-starter-pnpm-ts\src
[Map Iterator] {
  'D:/dev/source/gatsby/gatsby-starter-pnpm-ts/src/components/seo.tsx',
  'D:/dev/source/gatsby/gatsby-starter-pnpm-ts/src/layouts/main.tsx',
  'D:/dev/source/gatsby/gatsby-starter-pnpm-ts/src/components/image.tsx',
  'D:/dev/source/gatsby/gatsby-starter-pnpm-ts/node_modules/.pnpm/registry.npmjs.org/gatsby-transformer-sharp/2.3.13_04195f47e3c92d97e09f933485bf4df6/node_modules/gatsby-transformer-sharp/src/fragments.js' }

So, path.resolve() is returning non-posix compliant paths on Windows, while the trackedSource entries are (semi) posix compliant.

If I change that section to normalize componentPath:

  const pluginDocuments = Object.values(emitPluginDocuments).some(Boolean) && (
    stripIndent(
      Array.from(trackedSource.entries())
        .filter(([componentPath]) => !path.normalize(componentPath).startsWith(path.resolve(basePath, 'src')))
        .map(([, source]) => source.rawSDL)
        .join('\n'),
    )
  );

I get the desired results

Js-Brecht commented 4 years ago

I don't recall if I had this issue on Linux, as well. I'll have to check later

Js-Brecht commented 4 years ago

I just realized that I have to deal with outside of src to support gatsby-node. Thank you so much @Js-Brecht !

Out of curiosity, how do you plan to handle that? I ask because gatsby-plugin-ts-config could potentially conflict with that process, because of https://github.com/Js-Brecht/gatsby-plugin-ts-config/issues/2. If the path to gatsby-node is hard coded to the root of the project, then whoever uses gatsby-plugin-ts-config may not be able to use gatsby-plugin-typegen to generate types from queries in gatsby-node.

This won't be an issue if babel is used to do ahead-of-time transpiling, because it will be running the compiled gatsby-node out of a subdirectory in the cache (or somewhere similar), but if something is used that does JIT transpiling (like @babel/register), then gatsby-node will not be able to be stored in the project root without causing node ownership conflicts.

Perhaps this would be better as a separate issue? Do you have anything tracking this work?

cometkim commented 4 years ago

@Js-Brecht It is... hard to make it compatible with your project for now. (or please let me know if you have an idea)

The gatsby-* files in the project root, and the files in the src path and gatsby dependency, are what gatsby assumes for end-user projects and plugins/themes so it's easiest to follow that assumption.

I think you should talk more closely with the core team about that. It is likely to be included in the next breaking change.

Js-Brecht commented 4 years ago

It is... hard to make it compatible with your project for now. (or please let me know if you have an idea)

Would it be feasible to include an option allowing the user to define various additional files to pull types from? Perhaps that would be too labor intensive (and probably far too much maintenance), since I think it would require implementing Gatsby's query extraction apart from Gatsby itself. Correct me if I'm wrong.

The issues with node ownership will actually be resolved when Gatsby is able to read Typescript gatsby-* files natively, because then it is no problem keeping those files in the project root.

I think you should talk more closely with the core team about that. It is likely to be included in the next breaking change.

Will do, thank you

Js-Brecht commented 4 years ago

Would it be feasible to include an option allowing the user to define various additional files to pull types from? Perhaps that would be too labor intensive (and probably far too much maintenance), since I think it would require implementing Gatsby's query extraction apart from Gatsby itself.

I wonder if that could be as simple as using Gatsby's FileParser, like they do in the parseQueries function, to gather queries from custom file paths 🤔. Since executing the queries isn't necessarily an issue, running the FileParser should, at most, cause your GraphQL parser to run, because it fires the redux action.

The queries won't be watched for changes, but perhaps that could be arranged another way.

In the recent version of gatsby-plugin-ts-config, if the default export of the user's gatsby-config or gatsby-node is a function, then one of the parameters will be the collection of resolved user gatsby-* files. That would make it easy for the user to feed those particular files into your plugin for query typing. It could be left as just that basic collection, but it would be trivial to extend it to include the files that are require()'d by those files, too (in case they also include queries that aren't already parsed by Gatsby).

Js-Brecht commented 4 years ago

I just took a moment to read your code, instead of just skimming it, and I see you are already using the FileParser. So, I was just getting ahead of myself there ☝️

cometkim commented 4 years ago

Would it be feasible to include an option allowing the user to define various additional files to pull types from? Perhaps that would be too labor intensive (and probably far too much maintenance), since I think it would require implementing Gatsby's query extraction apart from Gatsby itself. Correct me if I'm wrong.

It can be easily done using graphql-tag-pluck. (Even I have a simple RegExp which can do almost the same thing as the Gatsby's file parser)

I just took a moment to read your code, instead of just skimming it, and I see you are already using the FileParser. So, I was just getting ahead of myself there

Anyway, as you saw, I switched to using Gatsby's action and the file parser instead of watching the user directory to extract the queries directly. Removing the custom watcher process was a big goal for v1, so it's hard to revert it at this moment.

It seems off-topic. @Js-Brecht Should we have another issue for this?

Js-Brecht commented 4 years ago

I will copy the conversation to a new issue