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

[templates/nextjs] [templates/nextjs-styleguide] Remove graphql-let and rely directly on graphql-codegen for graphql code generation #1713

Open
wants to merge 9 commits into
base: dev
Choose a base branch
from

Conversation

yavorsk
Copy link
Contributor

@yavorsk yavorsk commented Jan 15, 2024

Description / Motivation

This pull request removes graphql-let as a dependency and makes necessary changes so that we use directly graphql-codegen for graphql code generation. (based on fa37c60)
Reason for these changes being a./ graphql-let is not actively maintained and contains several critical security vulnerabilities and b./ it causes (or at least worsens) the problem with out of memory error during code generation. Changes include:

  • removes graphql-let from dependencies;
  • upgrades all graphql-codegen packages to latest;
  • adds the necessary graphql-codegen configuration and update to package json scripts;

Several notes:

  • graphql-codegen does not create .d.ts files as graphql-let does. Instead queries should be imported from the main generated file so this is a breaking change;
  • graphql-codegen throws an error if configuration is set search for .graphql files but it does not find them in the specified path; this is why the 'documents' setting in the nextjs sample should be commented out and additional config file is added for nextjs-stylguide template;

Testing Details

  • Unit Test Added
  • Manual Test/Other (Please elaborate) - test graphql code generation - by adding new fields templates in sitecore; by adding graphql queries;

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

@@ -71,7 +72,7 @@
"scripts": {
"jss": "jss",
"lint": "eslint ./src/**/*.tsx ./src/**/*.ts ./scripts/**/*.ts",
"bootstrap": "ts-node --project tsconfig.scripts.json scripts/bootstrap.ts && graphql-let",
"bootstrap": "ts-node --project tsconfig.scripts.json scripts/bootstrap.ts && jss graphql-codegen",
Copy link
Contributor

Choose a reason for hiding this comment

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

would it be better to use npm graphql-codegen here? jss is just an alias for npm.
Or better yet - is it possible to add graphql init logic into bootstrap script?

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 left it here because I think it would be easier for the user to disable code generation at build time if needed. Do you think it would be better to be in the script?

@@ -49,6 +50,7 @@
"@types/react-dom": "^18.0.5",
"@typescript-eslint/eslint-plugin": "^5.49.0",
"@typescript-eslint/parser": "^5.49.0",
"@tanstack/react-query": "^5.17.9",
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is this dependency used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is referenced by the generated file

Copy link
Contributor

Choose a reason for hiding this comment

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

@yavorsk Should you use this package instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's still referenced here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the two packages are different: @graphql-codegen/typescript-react-query is a plugin used during code generation; @tanstack/react-query is referenced by the generated ts file;

@@ -0,0 +1,9 @@
overwrite: true
# documents: 'src/**/*.graphql'
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a comment explaining why is this commented out, and when devs might need to enable it?

Copy link
Contributor

Choose a reason for hiding this comment

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

@yavorsk If it will be disabled by default, it means that in case we scaffold nextjs-styleguide - graphql code generation will not be executed, so the error will be thrown in GraphQL-ConnectedDemo, did you test that?

Copy link
Contributor

Choose a reason for hiding this comment

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

@yavorsk I see, so you duplicated a config file, is it possible to override npm command instead by providing documents flag? If it's possible. In order to don't copy/paste the config

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The library does not provide documents flag. There is a config flag, which specifies the config file to be used, but I am not sure if that would be cleaner in our case...?

@illiakovalenko
Copy link
Contributor

illiakovalenko commented Jan 17, 2024

@yavorsk I noticed a recommendation added by graphql-codegen to use client-preset package to reduce a bundle size and improve dev experience: https://the-guild.dev/graphql/codegen/plugins/typescript/typescript-react-query
Did you explore this option? It should be possible to use in React/Vue based projects. So, it makes sense for us

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

Successfully merging this pull request may close these issues.

3 participants