eslint-plugin-graphql icon indicating copy to clipboard operation
eslint-plugin-graphql copied to clipboard

Handle spread fragments for the required fields

Open arnmishra opened this issue 4 years ago • 5 comments
trafficstars

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:

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

arnmishra avatar Mar 10 '21 23:03 arnmishra

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

apollo-cla avatar Mar 10 '21 23:03 apollo-cla

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.

potch avatar May 10 '21 22:05 potch

@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 avatar May 10 '21 23:05 arnmishra

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

arnmishra avatar May 10 '21 23:05 arnmishra

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

Have you looked at graphql-eslint?

Urigo avatar May 12 '21 18:05 Urigo