Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[unused-fields] Add option to check leave fields only #95

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

janicduplessis
Copy link
Contributor

@janicduplessis janicduplessis commented May 16, 2020

Builds on top of #94 for option parsing code. Check commit 8d0773f only.

Motivation

This makes it so we only check leave fields, I'm not sure if there is actually cases where checking non-leaves is useful, so I left it as an option. If there are not we could make this the default. There are cases where non leave fields are never accessed because of helper functions in other files.

For example I use a useConnectionArray hook to transform a connection to an array. This will currently trigger the unused field rule since the current file never accesses edges and node.

Example:

graphql`fragment TestNested on Page { connection { edges { node { id } } } }`;
...
const nodes = useConnectionArray(props.connection);
...
nodes[0].id

// error: unused edges, node

Test plan

Tested on a large relay codebase and added tests.

@jscheid
Copy link

jscheid commented Oct 1, 2020

+1, we have the same problem. We'd like to use a helper to extract nodes from a connection and it generates false positives. The reasoning seems sound - if leaves are being used then the intermediary nodes are being used implicitly, even if the code can't detect it reliably. It would be great if this could get merged.

Copy link

@sibelius sibelius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very common pattern

Comment on lines +132 to +138
{
code: `
graphql\`fragment Test on Page { unused1 { unused2 } }\`;
`,
options: [{leavesOnly: true}],
errors: [unusedFieldsWarning('unused2')]
},
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does it works well if we use options, for some reason, as [{leavesOnly: true}, ignoreFields: ['unsed2']]?
I think that can add a new test

{
  code: `
    graphql\`fragment Test on Page { unused1 { unused2 } }\`;
  `,
  options: [{leavesOnly: true}, ignoreFields: ['unsed2']],
},

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants