dotansimha / dotansimha/graphql-code-generator
`--watch` mode does not filter out file change events that are negated as part of a set of patterns
- Dominant language
- TypeScript
- Stars
- 11.3k
- Forks
- 1.4k
- Avg merge
- 1d 1h
- Merged PRs (30d)
- 23
Description
### Which packages are impacted by your issue?
@graphql-codegen/cli
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:
```yml
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:
```ts
if (!mm.contains(path, files)) return;
```
From the [documentation of `micromatch.contains`](https://github.com/micromatch/micromatch#contains):
> Returns true if the given string contains the given pattern. Similar to [.isMatch](https://github.com/micromatch/micromatch#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):
```diff
- 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:
```js
// 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._
```js
// 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:
```js
// 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_!
```js
// 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:
```js
// 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:
```js
> 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
- `graphql` version: 15.8.0
- `@graphql-codegen/cli` version(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 in `3.2.2` but we just didn't have our `.next` directory 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`
Contributor guide
No contributing guide indexed for this repository
Assessment
This issue has not been assessed yet.