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.

Sign up for GitHub

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

fix: regeneration should only happen for config document pattern #9051

Merged
merged 2 commits into from
Feb 23, 2023
Merged

fix: regeneration should only happen for config document pattern #9051

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
254177b
fix: regeneration should only happen for config document pattern
saihaj Feb 23, 2023
66a48bd
chore(dependencies): updated changesets for modified dependencies
github-actions[bot] Feb 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/@graphql-codegen_cli-9051-dependencies.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@graphql-codegen/cli": patch
---
dependencies updates:
- Added dependency [`micromatch@^4.0.5` ↗︎](https://www.npmjs.com/package/micromatch/v/4.0.5) (to `dependencies`)
5 changes: 5 additions & 0 deletions .changeset/honest-spiders-rush.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@graphql-codegen/cli': patch
---

only run generate for files that users have listed in config to avoid running this over every change that codegen is not supposed to execute
2 changes: 2 additions & 0 deletions packages/graphql-codegen-cli/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@
"json-to-pretty-yaml": "^1.2.2",
"listr2": "^4.0.5",
"log-symbols": "^4.0.0",
"micromatch": "^4.0.5",
"shell-quote": "^1.7.3",
"string-env-interpolation": "^1.0.1",
"ts-log": "^2.2.3",
Expand All @@ -82,6 +83,7 @@
"@types/inquirer": "8.2.6",
"@types/is-glob": "4.0.2",
"@types/js-yaml": "4.0.5",
"@types/micromatch": "^4.0.2",
"@types/mkdirp": "1.0.2",
"@types/shell-quote": "1.7.1",
"bdd-stdin": "0.2.0",
Expand Down
7 changes: 7 additions & 0 deletions packages/graphql-codegen-cli/src/utils/watcher.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { isValidPath } from '@graphql-tools/utils';
import type { subscribe } from '@parcel/watcher';
import debounce from 'debounce';
import isGlob from 'is-glob';
import mm from 'micromatch';
import logSymbols from 'log-symbols';
import { executeCodegen } from '../codegen.js';
import { CodegenContext, loadContext } from '../config.js';
Expand Down Expand Up @@ -95,6 +96,12 @@ export const createWatcher = (
// it doesn't matter what has changed, need to run whole process anyway
await Promise.all(
events.map(async ({ type: eventName, path }) => {
/**
* @parcel/watcher has no way to run watcher on specific files (https://github.com/parcel-bundler/watcher/issues/42)
* But we can use micromatch to filter out events that we don't care about
*/
if (!mm.contains(path, files)) return;
Copy link

Choose a reason for hiding this comment

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

@saihaj I believe there's still an issue here when a negated path is used.

My use case looks like this:

documents:
  - './apps/frontend/src/app/**/*.graphql'
  - '!./apps/frontend/src/app/exclude-these/**/*.graphql'

I'm seeing the watcher run when any file in my source tree is changed, and I'm thinking it's due to '!./apps/frontend/src/app/exclude-these/**/*.graphql' causing the mm.contains call to return true.

Quick example with mm:

const mm = require('micromatch');

// Prints false
console.log(mm.contains('anyfile.js', './some-dir/**/*.graphql'));

// Prints true
console.log(mm.contains('anyfile.js', '!./some-dir/**/*.graphql'));

Copy link

Choose a reason for hiding this comment

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

I haven't checked the code, but have noticed an excessive amount of runs in watch mode since the update myself.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If you can open issue with repro will be easier for me to debug and fix

Copy link

@ChristianIvicevic ChristianIvicevic Mar 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I have downgraded to 3.1.0 and noticed that there is indeed a regression in newer versions causing the generator to run 5-10 times in a row (probably tracking changes in the .next folder). For now in 3.2+ I had to remove all negated paths and be more specific with included directories which helped to reduce the amount of compilations.

Unfortunantely my project is not public and I don't necesarily have sufficient time to reduce it to a MWE, but I could consider sharing the repository with you @saihaj in case you'd be willing to look into it unless somebody has a decent reproduction.

Copy link

Choose a reason for hiding this comment

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

A minor update: I found the --debug flag and confirmed that having negations causes them to be picked up causing the excess updates:
CleanShot 2023-03-11 at 16 46 27

Copy link

Choose a reason for hiding this comment

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

I did a similar investigation by using --debug and modifying the source directly and can confirm the same thing that Christian is seeing. I was able to remove the negated pattern and now things work as I want them do.

Fortunately, our project doesn't need the negated pattern - I only added the negated pattern with v3.2.0 to try to fix the issue that this PR ended up fixing. I am sharing this to confirm the behavior that the other user is also seeing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's a great find! Let's use this cause this PR notif got hidden in my inbox #9177

Copy link
Contributor

Choose a reason for hiding this comment

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

We have the exact same issue in our Next app, now tracking it here #9270 (instead of #9177)

Think I found the issue/root cause, working on a fix now


lifecycleHooks(config.hooks).onWatchTriggered(eventName, path);
debugLog(`[Watcher] triggered due to a file ${eventName} event: ${path}`);
const fullPath = join(process.cwd(), path);
Expand Down
42 changes: 13 additions & 29 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4430,6 +4430,11 @@
dependencies:
"@types/node" "*"

"@types/braces@*":
version "3.0.1"
resolved "https://registry.yarnpkg.com/@types/braces/-/braces-3.0.1.tgz#5a284d193cfc61abb2e5a50d36ebbc50d942a32b"
integrity sha512-+euflG6ygo4bn0JHtn4pYqcXwRtLvElQ7/nnjDu7iYG56H0+OhCd7d6Ug0IE3WcFpZozBKW2+80FUbv5QGk5AQ==

"@types/connect-history-api-fallback@^1.3.5":
version "1.3.5"
resolved "https://registry.yarnpkg.com/@types/connect-history-api-fallback/-/connect-history-api-fallback-1.3.5.tgz#d1f7a8a09d0ed5a57aee5ae9c18ab9b803205dae"
Expand Down Expand Up @@ -4654,6 +4659,13 @@
resolved "https://registry.yarnpkg.com/@types/mdx/-/mdx-2.0.3.tgz#43fd32414f17fcbeced3578109a6edd877a2d96e"
integrity sha512-IgHxcT3RC8LzFLhKwP3gbMPeaK7BM9eBH46OdapPA7yvuIUJ8H6zHZV53J8hGZcTSnt95jANt+rTBNUUc22ACQ==

"@types/micromatch@^4.0.2":
version "4.0.2"
resolved "https://registry.yarnpkg.com/@types/micromatch/-/micromatch-4.0.2.tgz#ce29c8b166a73bf980a5727b1e4a4d099965151d"
integrity sha512-oqXqVb0ci19GtH0vOA/U2TmHTcRY9kuZl4mqUxe0QmJAlIW13kzhuK5pi1i9+ngav8FjpSb9FVS/GE00GLX1VA==
dependencies:
"@types/braces" "*"

"@types/mime@*":
version "3.0.1"
resolved "https://registry.yarnpkg.com/@types/mime/-/mime-3.0.1.tgz#5f8f2bca0a5863cb69bc0b0acd88c96cb1d4ae10"
Expand Down Expand Up @@ -8135,35 +8147,7 @@ esbuild@^0.16.14:
"@esbuild/win32-ia32" "0.16.17"
"@esbuild/win32-x64" "0.16.17"

esbuild@^0.17.6:
version "0.17.8"
resolved "https://registry.yarnpkg.com/esbuild/-/esbuild-0.17.8.tgz#f7f799abc7cdce3f0f2e3e0c01f120d4d55193b4"
integrity sha512-g24ybC3fWhZddZK6R3uD2iF/RIPnRpwJAqLov6ouX3hMbY4+tKolP0VMF3zuIYCaXun+yHwS5IPQ91N2BT191g==
optionalDependencies:
"@esbuild/android-arm" "0.17.8"
"@esbuild/android-arm64" "0.17.8"
"@esbuild/android-x64" "0.17.8"
"@esbuild/darwin-arm64" "0.17.8"
"@esbuild/darwin-x64" "0.17.8"
"@esbuild/freebsd-arm64" "0.17.8"
"@esbuild/freebsd-x64" "0.17.8"
"@esbuild/linux-arm" "0.17.8"
"@esbuild/linux-arm64" "0.17.8"
"@esbuild/linux-ia32" "0.17.8"
"@esbuild/linux-loong64" "0.17.8"
"@esbuild/linux-mips64el" "0.17.8"
"@esbuild/linux-ppc64" "0.17.8"
"@esbuild/linux-riscv64" "0.17.8"
"@esbuild/linux-s390x" "0.17.8"
"@esbuild/linux-x64" "0.17.8"
"@esbuild/netbsd-x64" "0.17.8"
"@esbuild/openbsd-x64" "0.17.8"
"@esbuild/sunos-x64" "0.17.8"
"@esbuild/win32-arm64" "0.17.8"
"@esbuild/win32-ia32" "0.17.8"
"@esbuild/win32-x64" "0.17.8"

esbuild@~0.17.6:
esbuild@^0.17.6, esbuild@~0.17.6:
version "0.17.10"
resolved "https://registry.yarnpkg.com/esbuild/-/esbuild-0.17.10.tgz#3be050561b34c5dc05b46978f4e1f326d5cc9437"
integrity sha512-n7V3v29IuZy5qgxx25TKJrEm0FHghAlS6QweUcyIgh/U0zYmQcvogWROitrTyZId1mHSkuhhuyEXtI9OXioq7A==
Expand Down