graphql-code-generator: `--watch` mode does not filter out file change events that are negated as part of a set of patterns
Which packages are impacted by your issue?
EDIT: PR opened at #9275
Describe the bug
This is a follow-up to #9266 (which was fixed by #9267), but it’s an entirely separate bug. That fix just happened to expose the bug in our repository.
In our case, we have a NextJS app which uses the generated code from graphql-codegen, which means that on most page reloads, NextJS rebuilds pages in the .next directory. We noticed that these changes were not being filtered out of watch mode, even though they didn’t match any of our patterns:
Running lifecycle hook "onWatchTriggered" scripts...
[Watcher] triggered due to a file create event: /src/js/cloud/catalog/.next/cache/webpack/server-development/7.pack
Running lifecycle hook "onWatchTriggered" scripts...
[Watcher] triggered due to a file delete event: /src/js/cloud/catalog/.next/cache/webpack/server-development/7.pack_
Running lifecycle hook "onWatchTriggered" scripts...
[Watcher] triggered due to a file create event: /src/js/cloud/catalog/.next/cache/webpack/server-development/index.pack
Running lifecycle hook "onWatchTriggered" scripts...
[Watcher] triggered due to a file delete event: /src/js/cloud/catalog/.next/cache/webpack/server-development/index.pack_
These file names could not possibly match any of our patterns in codegen.yml, which are all either files ending with *.graphql or *.ts:
Our `codegen.yml` from our repository
This is not reproducible, but is just to show how there are no patterns that could possibly match the files triggering the rebuilds:
overwrite: true
config:
scalars:
Datetime: "string"
JSON: "{ [key: string]: any }"
UUID: "string"
documentMode: graphQLTag
autogeneratedFileWarning: &autogeneratedFileWarning
add: "/* DO NOT EDIT! This file is auto-generated by graphql-code-generator - see `codegen.yml` */"
generates:
types/index.ts:
schema:
- "../gql-api/schema/unified_schema.graphql"
documents:
- "queries/*.graphql"
# All .graphql files in catalog...
- "../catalog/**/*.graphql"
# ...except those that are compiled into types/search.tsx in next section
- "!../catalog/search/**/*.graphql"
- "../dynamic-forms/scripts/fetch-externals.ts"
plugins:
- *autogeneratedFileWarning
- "typescript"
- "typescript-operations"
- "typed-document-node"
# types/search.tsx
# Typings search API, via apollo-server-micro mounted on catalog/pages/api
types/search.tsx:
schema:
# Note: /**/* will NOT include catalog/search/schema/withSearchServerConfig.js
- "../catalog/search/schema/**/*.ts":
# Need custom loader because we need side-effects to generate schema
loader: ./loaders/SearchLoader.js
# don't parse the AST of the files, since SearchLoader is handling them
noPluck: true
documents:
- "../catalog/search/**/*.graphql"
plugins:
- *autogeneratedFileWarning
- "typescript"
- "typescript-operations"
- "typed-document-node"
# Same as above, but for saving the schema to a file
# (it would probably better to put this in SearchLoader.js)
types/search-schema.graphql:
schema:
- "../catalog/search/schema/**/*.ts":
loader: ./loaders/SearchLoader.js
noPluck: true
plugins:
- schema-ast
# Types for the Splitgraph Console
types/console.tsx:
schema:
- "../gql-api/schema/unified_schema.graphql"
documents:
- "../console/**/*.graphql"
plugins:
- *autogeneratedFileWarning
- "typescript"
- "typescript-operations"
- "typed-document-node"
config:
avoidOptionals: true
As you can see, all patterns either end with .graphql or .ts, and yet the file change events are triggering on files that end with .pack.
I believe this bug was introduced in #9051 which added this code for filtering events:
if (!mm.contains(path, files)) return;
From the documentation of micromatch.contains:
Returns true if the given string contains the given pattern. Similar to .isMatch but the pattern can match any part of the string.
My first guess what that the correct fix is to change .contains to .isMatch, and in our repository at least, this does help somewhat, but there are some other issues (see below):
- if (!mm.contains(path, files)) return;
+ if (!mm.isMatch(path, files)) return;
I believe the issue comes down to negation, and how it’s handled when part of a set of matchers. It’s possible this is a micromatch bug, I’m not sure. Let’s look at some examples.
In our case, we have a set of patterns which should be evaluated together, that is, the negation pattern should not be evaluated on its own, but rather as a boolean AND with the other patterns.
First, this is how the current code evaluates, which is obviously wrong:
// This returns true, but we would expect it to return false because of the negation
require("micromatch").contains(
"/src/js/cloud/catalog/.next/cache/webpack/server-development/index.pack",
[
"queries/*.graphql",
"../catalog/**/*.graphql",
"../dynamic-forms/scripts/fetch-externals.ts",
"!../catalog/search/**/*.graphql",
]
);
// > true
If we remove the negation pattern, then it is false as we expect. This means that the reason .contains is evaluating to true is because it “matches” !../catalog/search/**/*.graphql. However, we want it to evaluate to false, because the negation pattern should only negate any matches from the rest of the patterns (i.e., the rules should be evaluated as a set, not individually). Note: I tried different orders of these rules and it made no difference.
// Removing the negation pattern makes it evaluate to false, indicating the problem is the negation pattern
require("micromatch").contains(
"/src/js/cloud/catalog/.next/cache/webpack/server-development/index.pack",
[
"queries/*.graphql",
"../catalog/**/*.graphql",
"../dynamic-forms/scripts/fetch-externals.ts",
]
);
// > true
So, can we use .isMatch instead? Well, you would think so, except this has the same issue:
// This also incorrectly evaluates to true
require("micromatch").isMatch(
"/src/js/cloud/catalog/.next/cache/webpack/server-development/index.pack",
[
"queries/*.graphql",
"../catalog/**/*.graphql",
"../dynamic-forms/scripts/fetch-externals.ts",
"!../catalog/search/**/*.graphql",
]
);
// > true
Why do I say “you would think so”? Well, because when calling the default export with a list of files, micromatch correctly evaluates the boolean as a set, and returns an empty list as expected!
// This returns an empty list [], because none of the items in the input list match any of the patterns
require("micromatch")(
["/src/js/cloud/catalog/.next/cache/webpack/server-development/index.pack"],
[
"queries/*.graphql",
"../catalog/**/*.graphql",
"../dynamic-forms/scripts/fetch-externals.ts",
"!../catalog/search/**/*.graphql",
]
);
// > []
That’s the behavior we want! And to be sure, let’s make sure it does match an actual matching file:
// This returns a matching item, as expected
require("micromatch")(
["../catalog/blah/blah/blah.graphql"],
[
"queries/*.graphql",
"../catalog/**/*.graphql",
"../dynamic-forms/scripts/fetch-externals.ts",
"!../catalog/search/**/*.graphql",
]
)
// [ '../catalog/blah/blah/blah.graphql' ]
However, there is another problem here, which is that it only works when there is a relative path provided. When providing an absolute path, and even when explicitly setting cwd, it does not work as expected:
> process.cwd()
'/src/js/cloud/graphql'
// This should return a matching item, but returns an empty list instead
require("micromatch")(
["/src/js/cloud/catalog/blah/blah/blah.graphql"],
[
"queries/*.graphql",
"../catalog/**/*.graphql", // It should match this one
"../dynamic-forms/scripts/fetch-externals.ts",
"!../catalog/search/**/*.graphql",
],
{ cwd: "/src/js/cloud/graphql" }
);
Your Example Website or App
no minimal reproduction atm, sorry
Steps to Reproduce the Bug or Issue
Sorry, no minimal reproduction atm, but I do have example micromatch rules failing in the description above.
Expected behavior
As a user, I expect that only file change events matching my specified patterns will trigger a rebuild, and I expect that a negation pattern will negate any other patterns in its same set. However, the bug is that the negation pattern is being evaluated “by itself” which means it’s matching a bunch of irrelevant files.
Screenshots or Videos
No response
Platform
- OS: Linux
- NodeJS: 16.13.1
graphqlversion: 15.8.0@graphql-codegen/cliversion(s):3.3.1-rc-20230404030530-47bcfb095, but that’s only because the bug was exposed once we were listening in our root directory; it was present in3.2.2but we just didn’t have our.nextdirectory within the same package as our codegen config
Codegen Config File
See above description
Additional context
I’ll try to fix this in a PR, but I’m still unclear if the bug is in here or micromatch
About this issue
- Original URL
- State: open
- Created a year ago
- Comments: 16 (12 by maintainers)
Commits related to this issue
- fix(watch): Remove double-concatenated `fullPath`, since `path` is already absolute Parcel Watcher always provides `path` as an absolute path (see the readme for the package), and we were incorrectly... — committed to milesforks/graphql-code-generator by milesrichardson a year ago
- fix(watch): Respect negation and precedence when triggering rebuilds Implement the algorithm for deciding when to trigger a rebuild during watch mode, in such a way that precedence (both in terms of ... — committed to milesforks/graphql-code-generator by milesrichardson a year ago
- fix(watch): Respect negation and precedence when triggering rebuilds Implement the algorithm for deciding when to trigger a rebuild during watch mode, in such a way that precedence (both in terms of ... — committed to milesforks/graphql-code-generator by milesrichardson a year ago
- fix(watch): Respect negation and precedence when triggering rebuilds Implement the algorithm for deciding when to trigger a rebuild during watch mode, in such a way that precedence (both in terms of ... — committed to milesforks/graphql-code-generator by milesrichardson a year ago
- fix(watch): Remove double-concatenated `fullPath`, since `path` is already absolute Parcel Watcher always provides `path` as an absolute path (see the readme for the package), and we were incorrectly... — committed to milesforks/graphql-code-generator by milesrichardson a year ago
- fix(watch): Respect negation and precedence when triggering rebuilds Implement the algorithm for deciding when to trigger a rebuild during watch mode, in such a way that precedence (both in terms of ... — committed to milesforks/graphql-code-generator by milesrichardson a year ago
- fix(watch): Remove double-concatenated `fullPath`, since `path` is already absolute Parcel Watcher always provides `path` as an absolute path (see the readme for the package), and we were incorrectly... — committed to milesforks/graphql-code-generator by milesrichardson a year ago
- fix(watch): Respect negation and precedence when triggering rebuilds Implement the algorithm for deciding when to trigger a rebuild during watch mode, in such a way that precedence (both in terms of ... — committed to milesforks/graphql-code-generator by milesrichardson a year ago
- Trigger rebuilds in watch mode while respecting rules of precedence and negation (#9275) * chore: Add two new `dev-test/star-wars` queries which can be excluded by micromatch patterns This commit ... — committed to dotansimha/graphql-code-generator by milesrichardson a year ago
I opened #9275 and left an initial self-review there, to point out what’s happening
Not sure, that seems like it might be too magical, at least without a flag to opt into it. I can’t speak to how other people use it, but we do some weird things with
.gitignoreand auto-generated files. For example, we have our autogenerated types in.gitignore, and we check in the schemas. So in our case it wouldn’t actually cause problems (since we don’t have any dependencies on.gitignorefiles). But I could imagine some people are auto-generating schemas, and keeping those in.gitignoretoo, and then auto-generating the types from the schemas.An opt-in flag seems reasonable though? But more important seems to be making visible to user why a match is happening, so maybe it would be nice if
--debugoutput included not only the file that it matched (as it does now), but also the pattern (or set of patterns!) that caused the match. But I think that would be maybe a bit too verbose in the case of pattern sets, so maybe it could just include the config path (e.g.generates["types/index.ts"]["schema"]["documents"]that it matched)?