Fragments are not properly detected for the `known-fragment-names` rule #472

jgoux · 2021-05-31T08:43:46Z

Hello all 👋,

Following the discussion here : #456 (comment)

Describe the bug

The rules known-fragment-names and no-unused-fragments are erroring even if all my fragments are loaded by graphql-eslint.

I edited the sources and logged the value of the siblings variable : https://github.com/dotansimha/graphql-eslint/blob/master/packages/plugin/src/sibling-operations.ts#L83-L88

When I run eslint CLI, here is the output : https://gist.github.com/jgoux/73b79e57b96531f89ab6c99b089c2512
You can see the errors at the end : https://gist.github.com/jgoux/73b79e57b96531f89ab6c99b089c2512#file-log-sh-L796-L818
As an example, the ProfileForm_user fragment which is in error is properly detected and loaded here : https://gist.github.com/jgoux/73b79e57b96531f89ab6c99b089c2512#file-log-sh-L98-L105

To Reproduce
Steps to reproduce the behavior:

Expected behavior

If all the fragments are detected by the plugin, I should have no errors in my case.

Environment:

OS: MacOS Big Sur
@graphql-eslint/...: 1.0.1
NodeJS: v14.15.5

Additional context

The text was updated successfully, but these errors were encountered:

dimaMachina · 2021-06-03T09:32:09Z

Hi @jgoux, please provide a sample repo and not output in gist.github.com

dimaMachina · 2021-07-11T23:59:59Z

Closing due to inactivity. Please feel free to reopen if you still need help.

jgoux · 2021-08-04T12:18:33Z

Sorry for taking so long! 😅

Here is the minimal repro : https://github.com/jgoux/graphql-eslint-repro

You need to run yarn & yarn lint and you will have these two errors :

It's still present on the last release 2.0.1

I don't see any option to reopen the issue.

Urigo · 2021-08-05T12:55:12Z

Hi @jgoux and thank you for the report

Sorry but I'm not adding a lot here but just labeling it according to our new Contribution Guide and issue flow.

It seems already got into stage 1 thanks to your reproduction! Thank you for that!

Now in order to advance to stage 2 we'll need a failing test, would be great if someone could help progress the issues through the stages.

Thank you and sorry that this comment is not a complete solution (yet).

dimaMachina · 2021-08-05T13:11:45Z

@jgoux I don’t see import comment in your file https://github.com/jgoux/graphql-eslint-repro/blob/main/src/UserProfileScreen.graphql

can you add #import './ProfileForm.graphql' at the top of UserProfileScreen.graphql file and lint it again? (I am on vacation and not able to test your reproduction on my machine)

jgoux · 2021-08-06T13:17:21Z

@B2o5T Indeed, it works with the #import './ProfileForm.graphql' statement, well done!

A couple of things :

Why is this needed if we already provide the operations option as stated here : Support #import #385 (comment). I thought the operations are turned into some kind of global registry in which graphql-eslint could lookup for the fragments presence.
It seems like the import syntax implemented for graphql-eslint has a little bug, we can't put a space character before the import keyword like # import './ProfileForm.graphql'. (which should be valid according to the doc here : https://www.graphql-tools.com/docs/schema-loading#using-import-expression)

mogelbrod · 2021-10-08T08:12:44Z

I just discovered this great project and had the same experience as @jgoux. From reading the docs I assumed the operations field removed the need to add import statements to the graphql files. Other than that everything appears to work well!

3nvi · 2021-11-26T16:00:52Z

I also agree with the above. Just added it to my project and it's amazing.

The only downside is that I have to disable those 2 rules around Fragments, cause I don't want to add unnecessary #import ... comments in my *.graphql operation files just to satisfy the linter

jgoux · 2021-12-01T10:39:06Z

Happy to see that I'm not the only one surprised by the behaviour with fragments and imports. 😄

Is it possible to have a clear answer to my first point @dotansimha ?

Why is this needed if we already provide the operations option as stated here : Support #import #385 (comment). I thought the operations are turned into some kind of global registry in which graphql-eslint could lookup for the fragments presence.

Is it a bug or the expected behaviour?

As we don't have auto-completion on graphql imports and no refactoring tools, it's pretty complicated to have the right paths for the fragments files (a lot of ../../../../..).

dimaMachina · 2021-12-05T15:42:51Z

Is it a bug or the expected behavior

@jgoux Currently is expected behavior. Providing operations allows loading all operations in cache before linting because some rules require that like no-unused-fields require to determine which fields are unused based on all used operations, or unique-fragment-name and unique-operation-name to ensure the name is unique across all operations.

The benefit of #import comments is to know exactly which fragment you import, also if you use graphql-tag/loader you'll need to import only 1 document instead of all split documents in your js/ts file.

Also, I have another example to show why #import is preferable to operations.

Let's imagine you have 3 documents

users.gql

#import 'user-fields.gql'
query {
  users {
     ...UserFields
  }
}

user-fields.gql

fragment UserFields on User {
  ...
}

another-user-fields.gql

fragment UserFields on User { # `no-unused-fragments` rule will report an error for that unused fragment
  ...
}

And for the completion, I'm okay to completely remove necessary for #import comments in favor of operations.

dimaMachina · 2021-12-09T00:08:53Z

@jgoux @3nvi @mogelbrod the necessary of using #import comments was removed, try 3.2.0-alpha-2e742a6.0 alpha version and give your feedback

mogelbrod · 2021-12-09T08:36:32Z

@B2o5T: Tried the alpha release and it indeed looks like the plugin is now picking up fragments from other files without the #import statements, nice work!

Unfortunately I still get Unknown fragment "Y" when linting a file like the one below:

query MainQuery {
  rootField { ...X }
}

Where X is defined in another file:

fragment X on Something {
  id name
  ...on SomethingSpecific { ...Y }
}

fragment Y on SomethingSpecific {
  more
}

This happens whether I include #import ... in the first file or not though. If I remove the fragment nesting it works.

dimaMachina · 2021-12-09T08:43:47Z

Thanks for the feedback! I’ll look later to fix that problem 🙂

dimaMachina · 2021-12-09T12:38:47Z

@mogelbrod ~~just tried to add a failing test with your example but everything looks fine, no errors report from graphql-eslint 🤔. Could you create a reproduction repo with an alpha version?~~

my fixtures

user.gql

query {
  user {
    ...UserFields
  }
}

user-fields.gql

fragment UserFields on User {
  id
  posts {
    ...on Post {
      ...PostFields
    }
  }
}

fragment PostFields on Post {
  id
}

UPDATE: found an issue, no need for repro 😄

dimaMachina · 2021-12-09T13:22:28Z

@mogelbrod fixed in 3.2.0-alpha-6aa2721.0, the related test was added also

mogelbrod · 2021-12-09T14:21:59Z

@mogelbrod fixed in 3.2.0-alpha-6aa2721.0, the related test was added also

That seems to have solved the nested fragments issue, nice!
It appears that specifying undefined fragments causes stack overflows in this version though:

Oops! Something went wrong! :(
ESLint: 7.25.0
RangeError: Maximum call stack size exceeded
Occurred while linting .../src/routes/Library.graphql:0
    at typeFromAST (.../node_modules/graphql/utilities/typeFromAST.js:16:21)
    at typeFromAST (.../node_modules/graphql/utilities/typeFromAST.js:25:17)
    at TypeInfo.enter (.../node_modules/graphql/utilities/TypeInfo.js:186:56)
    at Object.enter (.../node_modules/graphql/utilities/TypeInfo.js:383:16)
    at Object.visit (.../node_modules/graphql/language/visitor.js:200:21)
    at getFragmentDefsAndFragmentSpreads (.../node_modules/@graphql-eslint/eslint-plugin/index.js:374:13)
    at getMissingFragments (.../node_modules/@graphql-eslint/eslint-plugin/index.js:378:47)
    at handleMissingFragments (.../node_modules/@graphql-eslint/eslint-plugin/index.js:382:30)
    at handleMissingFragments (.../node_modules/@graphql-eslint/eslint-plugin/index.js:396:20)
    at handleMissingFragments (.../node_modules/@graphql-eslint/eslint-plugin/index.js:396:20)

dimaMachina · 2021-12-11T13:24:16Z

@mogelbrod can't reproduce 🤷‍♂️, what do you mean by undefined fragments. Could you provide a code example or reproduction repo?

mogelbrod · 2021-12-12T01:08:39Z

I was able to consistently repro it in an un-shareable business environment, but would definitely like to try reproducing it in a csb or similar - do you have a preferred template I can use?

And in case it would be helpful: by undefined fragment I was referring to a fragment name that didn't exist in any of the operation files.

dimaMachina · 2021-12-12T02:10:36Z

And in case it would be helpful: by undefined fragment I was referring to a fragment name that didn't exist in any of the operation files.

~~Still can't reproduce it with undefined fragment by any operation.~~

UPDATE: was able to reproduce, will fix it soon

UPDATE 2: fixed in 3.2.0-alpha-4ca7218.0

dimaMachina added the stage/0-issue-prerequisites Needs more information before we can start working on it label Jun 4, 2021

dimaMachina closed this as completed Jul 11, 2021

dimaMachina reopened this Aug 4, 2021

dimaMachina added stage/1-reproduction A reproduction exists and removed stage/0-issue-prerequisites Needs more information before we can start working on it labels Aug 4, 2021

dimaMachina mentioned this issue Nov 7, 2021

Bug with import when using default export and there is a space before import and after # ardatan/graphql-tools#3837

Open

dotansimha assigned dimaMachina Nov 28, 2021

dimaMachina mentioned this issue Dec 8, 2021

feat: remove the need of #import comments in favor of parserOptions.operations #832

Merged

dimaMachina added stage/5-alpha-release-testing The pull request is merged, an alpha release is available, to be tested and removed stage/1-reproduction A reproduction exists labels Dec 9, 2021

dimaMachina closed this as completed in #832 Dec 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fragments are not properly detected for the `known-fragment-names` rule #472

Fragments are not properly detected for the `known-fragment-names` rule #472

jgoux commented May 31, 2021

dimaMachina commented Jun 3, 2021

dimaMachina commented Jul 11, 2021

jgoux commented Aug 4, 2021 •

edited

Loading

Urigo commented Aug 5, 2021

dimaMachina commented Aug 5, 2021 •

edited

Loading

jgoux commented Aug 6, 2021 •

edited

Loading

mogelbrod commented Oct 8, 2021

3nvi commented Nov 26, 2021

jgoux commented Dec 1, 2021 •

edited

Loading

dimaMachina commented Dec 5, 2021

dimaMachina commented Dec 9, 2021

mogelbrod commented Dec 9, 2021 •

edited

Loading

dimaMachina commented Dec 9, 2021

dimaMachina commented Dec 9, 2021 •

edited

Loading

dimaMachina commented Dec 9, 2021

mogelbrod commented Dec 9, 2021

dimaMachina commented Dec 11, 2021

mogelbrod commented Dec 12, 2021 •

edited

Loading

dimaMachina commented Dec 12, 2021 •

edited

Loading

Fragments are not properly detected for the known-fragment-names rule #472

Fragments are not properly detected for the known-fragment-names rule #472

Comments

jgoux commented May 31, 2021

dimaMachina commented Jun 3, 2021

dimaMachina commented Jul 11, 2021

jgoux commented Aug 4, 2021 • edited Loading

Urigo commented Aug 5, 2021

dimaMachina commented Aug 5, 2021 • edited Loading

jgoux commented Aug 6, 2021 • edited Loading

mogelbrod commented Oct 8, 2021

3nvi commented Nov 26, 2021

jgoux commented Dec 1, 2021 • edited Loading

dimaMachina commented Dec 5, 2021

dimaMachina commented Dec 9, 2021

mogelbrod commented Dec 9, 2021 • edited Loading

dimaMachina commented Dec 9, 2021

dimaMachina commented Dec 9, 2021 • edited Loading

dimaMachina commented Dec 9, 2021

mogelbrod commented Dec 9, 2021

dimaMachina commented Dec 11, 2021

mogelbrod commented Dec 12, 2021 • edited Loading

dimaMachina commented Dec 12, 2021 • edited Loading

Fragments are not properly detected for the `known-fragment-names` rule #472

Fragments are not properly detected for the `known-fragment-names` rule #472

jgoux commented Aug 4, 2021 •

edited

Loading

dimaMachina commented Aug 5, 2021 •

edited

Loading

jgoux commented Aug 6, 2021 •

edited

Loading

jgoux commented Dec 1, 2021 •

edited

Loading

mogelbrod commented Dec 9, 2021 •

edited

Loading

dimaMachina commented Dec 9, 2021 •

edited

Loading

mogelbrod commented Dec 12, 2021 •

edited

Loading

dimaMachina commented Dec 12, 2021 •

edited

Loading