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

Required fields and fragments #199

Merged
merged 9 commits into from
Nov 13, 2018
Merged

Required fields and fragments #199

merged 9 commits into from
Nov 13, 2018

Conversation

stubailo
Copy link
Contributor

@stubailo stubailo commented Nov 12, 2018

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

Pull Request Labels

  • feature
  • blocking
  • docs

@stubailo stubailo changed the title [wip] Required fields and fragments Required fields and fragments Nov 13, 2018
@stubailo stubailo requested a review from jnwng November 13, 2018 07:13
@stubailo
Copy link
Contributor Author

cc @stevehollaar and @jnwng please give this a look if you have time! My intention is to make this rule a reliable way to ensure that a certain field is fetched (id in our case). @stevehollaar took the first step by being more conservative with named fragments, and I covered a few more cases with certain combinations of inline fragments.

Open to any ideas that allow more valid combinations of fields, but I'd also rather err on the conservative side.

@ghost ghost added blocking Prevents production or dev due to perf, bug, build error, etc.. feature New addition or enhancement to existing solutions labels Nov 13, 2018
@stubailo
Copy link
Contributor Author

cc @hellendag

CHANGELOG.md Outdated
- Here's a specific case which is _no longer valid_:
- `query { greetings { hello ... on Greetings { id } } }`
- This must now be written as `query { greetings { id hello ... on Greetings { id } } }`
- This is a more convervative approach than before, driven by the fact that it's quite hard to ensure that a combination of inline fragments actually covers all of the possible types of a selection set.
Copy link

Choose a reason for hiding this comment

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

nit: typo at "convervative"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for reviewing!

Copy link

@hellendag hellendag left a comment

Choose a reason for hiding this comment

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

🙌

type: "TaggedTemplateExpression"
}
]
},

Choose a reason for hiding this comment

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

Also worth testing an interface where id is a shared field?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

CHANGELOG.md Outdated
@@ -1,5 +1,13 @@
# Change log
### vNEXT
- BREAKING: The `required-fields` rule has been significantly changed to make it a completely reliable method of ensuring `id` fields are always requested when available. [PR #199](https://github.com/apollographql/eslint-plugin-graphql/pull/199) Here is the behavior, let's say we are requiring field `x`:
- On any field whose return type defines a field called `x`, the selection set must directly contain `x`.

Choose a reason for hiding this comment

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

x is a bit of an odd example field name. :D

});
},

// Every inline fragment must have an ID specified inside itself or in some

Choose a reason for hiding this comment

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

s/an ID/the required field

@stubailo
Copy link
Contributor Author

(Waiting on an OK from @jnwng about whether this breaking change is fine)

Copy link
Contributor

@jnwng jnwng left a comment

Choose a reason for hiding this comment

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

lgtm — this should definitely be a major bump

@stubailo stubailo merged commit a8c7dd5 into master Nov 13, 2018
@stubailo
Copy link
Contributor Author

Published as 3.0.0!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocking Prevents production or dev due to perf, bug, build error, etc.. feature New addition or enhancement to existing solutions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants