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

Support GraphQL Config in graphql-codegen plugin #645

Merged
merged 10 commits into from
May 20, 2024

Conversation

taro-28
Copy link
Contributor

@taro-28 taro-28 commented May 19, 2024

Added support for GraphQL Config in graphql-codegen plugin config file.
https://the-guild.dev/graphql/config

@webpro
Copy link
Collaborator

webpro commented May 19, 2024

Thanks for the PR, @taro-28! I'm wondering, what is the reason to extend the graphql-codegen plugin instead of creating a new plugin?

Their docs show the graphql-config package should enable this plugin, and so on. LEt me know if you have any questions, happy to answer them.

@taro-28
Copy link
Contributor Author

taro-28 commented May 19, 2024

@webpro

Thanks for your comment!
Indeed, it seems better to add the graphql-config plugin...!
(I added it to the graphql-codegen plugin without much thought...🙏)

I haven't fully understood how to add the plugin yet, but would it be okay if I tried it while looking at the existing code?

I am thinking of the following flow:

  1. If there is a graphql-config config file (e.g., graphql-config.ts) or graphql-config is included in the dependencies, enable the graphql-config plugin.
  2. Enable the plugin (e.g., graphql-codegen) corresponding to the key (e.g., graphql-codegen) in the extensions of the config file.

@webpro
Copy link
Collaborator

webpro commented May 19, 2024

I haven't fully understood how to add the plugin yet, but would it be okay if I tried it while looking at the existing code?

In case you've missed it, here's a guide to create the plugin including boilerplate generation: https://knip.dev/guides/writing-a-plugin

That said, of course you can also copy-paste an existing similar plugin.

The reason I thought it would be better to enable the graphql-config plugin based on the existence of a config file rather than whether graphql-config is included in the dependencies is that the graphql-config file can be used without being installed.

Do you mean that the config file is useful without the dependency/ies being installed? If so, do you maybe have an explanation or link about this?

@taro-28
Copy link
Contributor Author

taro-28 commented May 19, 2024

In case you've missed it, here's a guide to create the plugin including boilerplate generation: https://knip.dev/guides/writing-a-plugin

That said, of course you can also copy-paste an existing similar plugin.

Thank you very much. I will try with reference to those.

Do you mean that the config file is useful without the dependency/ies being installed? If so, do you maybe have an explanation or link about this?

Libraries like graphql-codegen will find the graphql-config configuration file.
https://github.com/dotansimha/graphql-code-generator/blob/21fbf0db2ba7a560aeb0aa52e9b9bf792ac94227/packages/graphql-codegen-cli/src/graphql-config.ts#L37-L49

So for users (not library developers), I think installing graphql-config is necessary only when they want to use types in a TypeScript configuration file like graphql.config.ts.
If they don't need to use types, they can use it without installation.

@taro-28 taro-28 marked this pull request as draft May 19, 2024 10:53
@webpro
Copy link
Collaborator

webpro commented May 19, 2024

Libraries like graphql-codegen will find the graphql-config configuration file.

Thanks for clarifying, now I see why you've extended the existing plugin. Having one plugin makes the implementation more complex, but I guess it's the way to go :)

I am thinking of the following flow:

  1. If there is a graphql-config config file (e.g., graphql-config.ts) or graphql-config is included in the dependencies, enable the graphql-config plugin.
  2. Enable the plugin (e.g., graphql-codegen) corresponding to the key (e.g., graphql-codegen) in the extensions of the config file.

How about using const enablers = [/^@graphql-codegen\//, "graphql-config"] and take it from there? I'm asking, because hasDependency is cheap, no disk reads for users without GQL.

Feel free to create multiple fixture folders to test the various use cases.

@taro-28
Copy link
Contributor Author

taro-28 commented May 19, 2024

hasDependency is cheap, no disk reads for users without GQL.

I see. You're right.
I'll try it that way!

Feel free to create multiple fixture folders to test the various use cases.

Thanks, all right :)

Copy link
Contributor Author

@taro-28 taro-28 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added graphql-config to enablers and added test cases.
Please review them at your convenience.

The CI is failing on integration with InvokeAI, and it seems to be caused by recent changes in invoke-ai/InvokeAI. How should I address this issue?

When I clone the repository and run the same commands as the CI, I encounter an error with knip-bun --production --fix --no-exit-code.

git clone https://github.com/invoke-ai/InvokeAI.git
cd InvokeAI/invokeai/frontend/web
pnpm install
pnpm exec knip-bun
# Error occurs
pnpm exec knip-bun --production --fix --no-exit-code

@@ -38,7 +38,7 @@ export type Resolve = (options: PluginOptions) => Promise<string[]> | string[];
export interface Plugin {
title: string;
enablers: IgnorePatterns | string;
packageJsonPath?: string;
packageJsonPath?: string | string[];
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines 260 to 265
get(
this.manifest,
(Array.isArray(packageJsonPath)
? packageJsonPath.find(path => get(this.manifest, path))
: packageJsonPath) ?? pluginName
)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would appreciate it, if there is a simpler way to do this…!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just an idea, maybe it would be slightly simpler if we would accept a function instead of an array, something like this:

typeof packageJsonPath === 'function'
  ? packageJsonPath(this.manifest)
  : (get(this.manifest, plugin.packageJsonPath) ?? pluginName)

Not much better, but at least any (additional) complexity is moved to the plugin.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

at least any (additional) complexity is moved to the plugin.

Certainly..., thank you...!
Fixed!
58f13a7

@taro-28 taro-28 marked this pull request as ready for review May 19, 2024 16:03
@webpro
Copy link
Collaborator

webpro commented May 19, 2024

The CI is failing on integration with InvokeAI, and it seems to be caused by recent changes in invoke-ai/InvokeAI. How should I address this issue?

Yeah this sometimes happens. I've "fixed" it in main, so if you rebase the lights should go green again.

@webpro
Copy link
Collaborator

webpro commented May 19, 2024

Pinging @beaussan, as you're the original author of the plugin. Any chance there's public repos using this plugin you could point us to so we can verify there are no regressions?

@taro-28 taro-28 force-pushed the graphql-codegen/support-graphql-config branch from 8310bf1 to 29382e0 Compare May 20, 2024 05:06
@taro-28
Copy link
Contributor Author

taro-28 commented May 20, 2024

Yeah this sometimes happens. I've "fixed" it in main, so if you rebase the lights should go green again.

Thanks, I rebased and it fixed it.

@taro-28 taro-28 requested a review from webpro May 20, 2024 15:54
@webpro
Copy link
Collaborator

webpro commented May 20, 2024

Excellent stuff, @taro-28! Going to merge it now and it'll be part of a bigger release, expected later this week.

@webpro webpro merged commit 8b12ded into webpro-nl:main May 20, 2024
11 of 14 checks passed
@taro-28
Copy link
Contributor Author

taro-28 commented May 20, 2024

@webpro
Thank you very much!
Your consistent and courteous support was very helpful!
Looking forward to the release!🤩

@taro-28 taro-28 deleted the graphql-codegen/support-graphql-config branch May 20, 2024 16:29
@beaussan
Copy link
Contributor

I've check everything and it looks great to me!

@webpro
Copy link
Collaborator

webpro commented May 20, 2024

I've check everything and it looks great to me!

Thank you so much, Nicolas! Didn't want to give you any more work so asked for repos, but this is even better! 🙏 Should go out later this week.

@webpro
Copy link
Collaborator

webpro commented May 22, 2024

🚀 This pull request is included in v5.17.0-canary.0. See Release 5.17.0-canary.0 for release notes.

Using Knip in a commercial project? Please consider sponsoring me.

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

Successfully merging this pull request may close these issues.

3 participants