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

[no-Jira] Run yarn gql:w alongside the Next.js dev server #926

Merged
merged 5 commits into from
May 2, 2024

Conversation

canac
Copy link
Contributor

@canac canac commented Apr 25, 2024

Description

  • Keep GraphQL generated files in sync with the source files by committing generated files.
  • Add a custom script to delete stale generated files that were left when a .graphql file was moved, renamed, or deleted. These stale files can cause TS errors and were a significant source of confusion.
  • Add a GitHub Action that will check that the generated files are up-to-date.
  • Speed up yarn gql by not running prettier on the generated files.
  • The files in src/graphql/ are not committed because their contents may differ depending on whether yarn gql was pointed at the staging or the production API server.
  • Remove post-checkout git hook because it is no longer needed.

Checklist:

  • I have given my PR a title with the format "MPDX-(JIRA#) (summary sentence max 80 chars)"
  • I have applied the appropriate labels. (Add the label "On Staging" to get the branch automatically merged into staging.)
  • I have requested a review from another person on the project

@canac canac added the On Staging Will be merged to the staging branch by Github Actions label Apr 25, 2024
@canac canac requested a review from dr-bizz April 25, 2024 16:39
Copy link
Contributor

github-actions bot commented Apr 25, 2024

Bundle sizes [mpdx-react]

Compared against dde4508

No significant changes found

@canac
Copy link
Contributor Author

canac commented Apr 25, 2024

@dr-bizz Sorry for another large PR. The vast majority of the files are yarn packages and generated files, which you should be able to ignore. But let me know if you want me to try to split it up more before you review.

Copy link
Contributor

@dr-bizz dr-bizz left a comment

Choose a reason for hiding this comment

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

Thank you for doing this. Your changes will make it easier for devs to switch between local branches.

I have left a few comments on how you can split up the changes more to ensure there is only one change per commit.
Can you also split this PR into two? One for updating graphql-codegen and the related files, and the other for .generated files and other commits. Having two separate PRs will ensure each PR is focused on one Conceptual change per PR.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
delete-stale-files.sh Outdated Show resolved Hide resolved
pages/GetAccountLists.generated.ts Outdated Show resolved Hide resolved
@canac canac changed the base branch from main to upgrade-gql-codegen April 25, 2024 19:19
@canac canac force-pushed the commit-gql-generated-files branch from 0d80faf to 0dc2785 Compare April 25, 2024 19:19
@canac
Copy link
Contributor Author

canac commented Apr 25, 2024

@dr-bizz I broke the work up into 2 PRs (see #928) and a lot of separate commits. Let me know if that's what you had in mind.

@canac canac requested a review from dr-bizz April 25, 2024 19:22
.github/workflows/ci.yml Outdated Show resolved Hide resolved
Base automatically changed from upgrade-gql-codegen to main April 26, 2024 15:26
Copy link
Contributor

@dr-bizz dr-bizz left a comment

Choose a reason for hiding this comment

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

Great work on this. I left a few comments.

codegen.yml Show resolved Hide resolved
codegen.yml Outdated Show resolved Hide resolved
@canac canac requested a review from dr-bizz April 26, 2024 16:11
Copy link
Contributor

@dr-bizz dr-bizz left a comment

Choose a reason for hiding this comment

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

Everything looks good so I'm approving this.

Please can you add the package script I mentioned and add some text to the ReadMe explaining why you need to run gql before pushing.

@canac canac force-pushed the commit-gql-generated-files branch from 38cc279 to dd02443 Compare April 29, 2024 21:25
@canac canac changed the title [no-Jira] Commit files generated by gql-codegen [no-Jira] Run yarn gql:w alongside the Next.js dev server Apr 29, 2024
@canac
Copy link
Contributor Author

canac commented Apr 29, 2024

I switched to a different approach of running yarn gql:w in parallel when you start the Next.js dev server. It should address all of the pain points of outdated generated files without needing to commit the generated files.

@canac canac requested a review from dr-bizz April 29, 2024 21:27
@canac canac force-pushed the commit-gql-generated-files branch from dd02443 to fd3b016 Compare May 1, 2024 15:36
@canac
Copy link
Contributor Author

canac commented May 1, 2024

I rewrote the bash delete stale files script in node so that it is more readable, maintainable, and robust. Specifically, it will handle filenames containing spaces correctly now.

@canac canac merged commit e75367d into main May 2, 2024
17 of 18 checks passed
@canac canac deleted the commit-gql-generated-files branch May 2, 2024 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
On Staging Will be merged to the staging branch by Github Actions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants