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

Handle spread fragments for the required fields #300

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

Conversation

arnmishra
Copy link

@arnmishra arnmishra commented Mar 10, 2021

Fix #248

The current required-fields rule only checks that the required fields exist on the top level fragment or query. However, if someone is using spread fragments, this doesn't recursively check all fragments.

The proposed solution in this PR only throws if the fragment or query has no spread fragment AND is missing the required field(s). This should work because at some point, the top-level spread fragment will be checked through the lint rule. As long as the top-level fragment has all required fields, all other fragments and queries that reference it will be covered as well.

First time contributor here, let me know if there are some explicit reasons not to include this. I tested it on my company's private repo and it worked as expected (and uncovered a few places an id was missing).

TODO:

  • Make sure all of the significant new logic is covered by tests
  • Rebase your changes on master so that they can be merged easily
  • Make sure all tests pass
  • Update CHANGELOG.md with your change
  • If this was a change that affects the external API, update the README (Not applicable)

@apollo-cla
Copy link

@arnmishra: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

@potch
Copy link

potch commented May 10, 2021

just ran into this- @arnmishra if you're able to sign the CLA above that would be awesome! If there's reasons (employer etc) you can't I'd be happy to see about signing and shepherding this patch.

@arnmishra
Copy link
Author

arnmishra commented May 10, 2021

@apollo-cla @potch Forgot to update here - pretty sure I signed the CLA back on 3/10. Just signed it again to be safe.

Edit: Yep, confirmed that I filled it out back on 3/10 (found the confirmation email). Maybe there's another step I need to take to get someone to merge this in.

Edit 2: Let me drop into the slack and see if I can get this merged

@arnmishra
Copy link
Author

arnmishra commented May 10, 2021

Posted on their Spectrum (https://spectrum.chat/apollo/contributing/looking-for-a-short-code-review-on-the-apollo-eslint-plugin~9f9c0022-f479-4b06-bbf9-a7d9d665d81b) which is what the repo's "slack" button redirects to but it seems like Spectrum has shut down for the most part. Also posted on a discord I saw linked on spectrum (https://discord.com/channels/733693158499549286/740949749028618280/841454114255667230). Let's see if we get a response

Edit: added to their new forum too https://community.apollographql.com/t/short-code-review-on-the-apollo-eslint-plugin/89

@Urigo
Copy link

Urigo commented May 12, 2021

@arnmishra @potch I'm not sure but I think this library is not actively maintained anymore..

Have you looked at graphql-eslint?

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

Successfully merging this pull request may close these issues.

graphql/required-fields rule ignores presence of required field in spread fragment
4 participants