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?

@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:

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
  • 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

About this issue

  • Original URL
  • State: open
  • Created a year ago
  • Comments: 16 (12 by maintainers)

Commits related to this issue

Most upvoted comments

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 .gitignore and 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 .gitignore files). But I could imagine some people are auto-generating schemas, and keeping those in .gitignore too, 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 --debug output 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)?