dimaMachina / graphql-eslint

ESLint parser, plugin and set rules for GraphQL (for schema and operations). Easily customizable with custom rules. Integrates with IDEs and modern GraphQL tools.
https://the-guild.dev/graphql/eslint
MIT License
795 stars 103 forks source link

Multi-project configurations cannot be linted all at once #2260

Open benasher44 opened 5 months ago

benasher44 commented 5 months ago

Issue workflow progress

Progress of the issue based on the Contributor Workflow


Describe the bug

The lint plugin does load per-project schemas, but it loads it once and ends up reusing it for all files. This results in using one project's schema to lint schemas of other projects.

To Reproduce Steps to reproduce the behavior:

  1. Configure multi-project graphql files setup (e.g. src/project1/*/.graphql, src/project2/*/.graphql)
  2. Run lint for the whole folder

Expected behavior

Lint should pass, but it instead claims that (for example) types are unreachable, since it's checking those types against the project that was loaded for the lint pass.

Environment:

Additional context

Having looked at the code a little bit, it looks like each rule looks at the loaded schema, which will be the schema loaded by the plugin at the start of the lint pass. Instead, each rule should reuse the code that loads the schema from the project-specific schema cache.

benasher44 commented 5 months ago

Maybe this is a FlatConfig-specific thing? I can't reproduce this in the example project, but I can definitely reproduce it in ours.

benasher44 commented 5 months ago

Confirmed this reproduces in the multiple-project sample project. It doesn't reproduce today because both schemas define a User type. If you just change one of them to be called AdminUser instead, it reproduces:

image

This will say that the AdminUser is unreachable, if eslint loads the other schema first.

user1736 commented 4 months ago

Also run into a similar issue. Any news on the fix?

user1736 commented 3 months ago

@dotansimha could you take a look into this? I can allocate some time into fixing it, but I need some guidance from the maintainers as to what the fix should look like.

The problem comes from here. Plugin caches the first config it reads and uses it for everything. If we were to extend the caching logic the question is what should be the key in that case. Using just path to schema is not enough, while using path to schema and all the operation documents seems like overkill 🤔 I tried to poke and see if it's possible to cache just schema and swap operation documents in and out but underlying classes that handle config are bad fit for it so it ended up being too hacky :(

dotansimha commented 3 months ago

@user1736 can you please share the patch fix you made locally? Or what's the suggested workaround for that? @dimaMachina can help with adjusting it.

dimaMachina commented 3 months ago

@user1736 hi, could you create a minimal reproduction with steps to reproduce issues with caching?

benasher44 commented 3 months ago

It's reproducible in the sample project in this repo. See this patch: https://github.com/dimaMachina/graphql-eslint/issues/2260#issuecomment-2070092469

user1736 commented 3 months ago

So for my case specifically I used the following patch:

diff --git a/node_modules/@graphql-eslint/eslint-plugin/cjs/graphql-config.js b/node_modules/@graphql-eslint/eslint-plugin/cjs/graphql-config.js
index 5a18a0a..00fe919
--- a/node_modules/@graphql-eslint/eslint-plugin/cjs/graphql-config.js
+++ b/node_modules/@graphql-eslint/eslint-plugin/cjs/graphql-config.js
@@ -37,7 +37,7 @@ var import_code_file_loader = require("@graphql-tools/code-file-loader");
 var import_debug = __toESM(require("debug"));
 var import_graphql_config = require("graphql-config");
 const debug = (0, import_debug.default)("graphql-eslint:graphql-config");
-let graphQLConfig;
+const configCache = new Map();
 function loadOnDiskGraphQLConfig(filePath) {
   return (0, import_graphql_config.loadConfigSync)({
     // load config relative to the file being linted
@@ -47,9 +47,12 @@ function loadOnDiskGraphQLConfig(filePath) {
     extensions: [codeFileLoaderExtension]
   });
 }
+
 function loadGraphQLConfig(options) {
-  if (graphQLConfig) {
-    return graphQLConfig;
+  // FIXME: Requires revisiting if we stop using parserOptions in eslintrc.js to feed the plugin schema and document files
+  const cacheKey = `${options.schema}#${options.documents?.length}`;
+  if (configCache.has(cacheKey)) {
+    return configCache.get(cacheKey);
   }
   const onDiskConfig = !options.skipGraphQLConfig && loadOnDiskGraphQLConfig(options.filePath);
   debug("options.skipGraphQLConfig: %o", options.skipGraphQLConfig);
@@ -64,13 +67,14 @@ function loadGraphQLConfig(options) {
     include: options.include,
     exclude: options.exclude
   };
-  graphQLConfig = onDiskConfig || new import_graphql_config.GraphQLConfig(
+  const graphQLConfig = onDiskConfig || new import_graphql_config.GraphQLConfig(
     {
       config: configOptions,
       filepath: "virtual-config"
     },
     [codeFileLoaderExtension]
   );
+  configCache.set(cacheKey, graphQLConfig);
   return graphQLConfig;
 }
 const codeFileLoaderExtension = (api) => {

For my configuration having schema name + number of documents is enough to distinguish between config sections, but it's not going to fly as general solution. From my testing disabling cache all together is also not an option since everything slows down to a crawl.

superpowered commented 2 months ago

Hello!

I've also just run into this issue on an internal company project.

Updating:

  if (process.env.NODE_ENV !== 'test' && graphQLConfig) {
    return graphQLConfig;
  }

to:

  if (process.env.NODE_ENV !== 'test' && graphQLConfig && graphQLConfig['_rawConfig'].schema === options.schema) {
    return graphQLConfig;
  }

Seems to be enough to fix it in our use-case at least. But I could see a test for both .schema and .documents matching to be a bit more accurate.

dimaMachina commented 2 months ago

@benasher44 can confirm that no-unreachable-types and no-unused-fields were broken for multi schema projects. Fixed in @graphql-eslint/eslint-plugin@4.0.0-alpha.2 https://github.com/dimaMachina/graphql-eslint/releases/tag/release-1722466173246