AP-682 Strip `FragmentDefinition`s in `client:check` when only included in `@client` fields by justinanastos · Pull Request #1454 · apollographql/apollo-tooling

justinanastos · 2019-07-31T21:39:18Z

We're getting client:check failures when using a Fragment in a @client field because while the fields with @client are stripped, Fragments that are nested below them are not. We now check that Fragments can be removed if they are only used in @client fields.

I added tests to the existing functionality on one commit, added a failing test for how I expected the method to behave, and then fixed the test. Please look at the last new test to see a reproduction.

TODO:

Update CHANGELOG.md* with your change (include reference to issue & this PR)
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 and linter rules pass

*Make sure changelog entries note which project(s) has been affected. See older entries for examples on what this looks like.

These will make sure we don't break the original functionality

…nitions If we remove a client field that includes fragment definitions, we should remove the fragment definition as well if that fragment isn't reused

…tatedFields This ensures that the type passed in extends from ASTNode and then returns the same type that is passed in, which may be a more specific version of ASTNode (like DocumentNode).

@client

…d in @client field If we spread a fragment inside of a `@client` field, we remove that fragment spread. If we don't use that fragment anywhere else then we should remove the fragment definition. It _is_ possible that a fragment is used on a @client field and non-client field if they are fragments on an interface and not and object.

jbaxleyiii

@justinanastos this is a good start, but fragments can be really tricky especially as the recurse. Happy to pair on this if you have questions about my comments!

justinanastos · 2019-08-02T16:17:36Z

@jbaxleyiii Great suggestions for new test cases and awesome explanations of what my first pass was missing.

I've added the suggested test cases and another one to make sure we don't remove fragments that we didn't remove because of @client directives. I made the removal recursive as well.

jbaxleyiii

This is getting closer! I left a few more questions where a couple more complex test cases with nested behaviors may be helpful to make sure all of the work is being done the way you want it to

jbaxleyiii · 2019-08-02T16:55:43Z

+        }
+
+        // All nested fragment spreads inside of this definition are now eligible to be removed
+        visit(ast, {


I don't think we should nest visitors here. This implementation doesn't do what I think you want it to anyway. By revisiting the root ast, instead of the specific node that you are in currently, you aren't "filtering" fragment spreads inside of this definition.

Since the visitor works depth first, you should be able to visit on FragmentDefinition to create your list, then visit the FragmentSpread next as long as you sort the document prior to doing the work so that fragment definitions always come first

Absolutely right.

This appears to still be nested? Why was this resolved?

I'm sorry about that; I have no idea. I'll figure this last piece out on Monday! Thanks again @jbaxleyiii !

My original logic was to visit all the descendents of node to find all fragment spreads being removed because this field was being removed as well. I wrote this to visit ast by accident; great catch @jbaxleyiii .

I think that I do, however, want to use a nested visitor here to flag all the fragment spreads under this node as eligible for deletion. I've added a comment to hopefully explain this. Please let me know if this is the correct way to handle this problem.

We're getting a list of all the fragment spreads and then removing all fragment spreads that aren't in the list we just created. This will never happen.

jbaxleyiii

@justinanastos this looks great except for the nesting of the visitors still?

jbaxleyiii · 2019-08-09T19:41:24Z

+/**
+ * Remove nodes that have zero-length selection sets
+ *
+ * The `ast` param must extend ASTNode. We use a genetic to indicate that this function returns the same type


jbaxleyiii · 2019-08-09T19:42:21Z

+        }
+
+        // All nested fragment spreads inside of this definition are now eligible to be removed
+        visit(ast, {


This appears to still be nested? Why was this resolved?

jbaxleyiii

@justinanastos thanks for sticking with me for the review cycles. I really appreciate the time and effort you put into this PR! I think it is good to go as is!

For future readers looking back, another approach that we could have taken here would be to look at the overall DocumentNode and collect 1) all fragments (inline and defined) 2) determine the relationships between them and the conditionality of that related to trimmable directives (i.e. @client) then re-walk over the AST using the visitor to selectively remove the listed fragments and trim their selection sets. This would be more kin to an "ahead of time" style versus a "just in time" style which this PR implements. If performance ever becomes a big concern, we may need to try the AOT but I'm happy with where this solution has landed

justinanastos requested review from jbaxleyiii and trevor-scheer July 31, 2019 21:39

justinanastos marked this pull request as ready for review July 31, 2019 21:42

justinanastos added 5 commits July 31, 2019 17:46

test: add removeDirectiveAnnotatedFields tests

8b51c93

These will make sure we don't break the original functionality

test: add removeDirectiveAnnotatedFields test to remove fragment defi…

7755934

…nitions If we remove a client field that includes fragment definitions, we should remove the fragment definition as well if that fragment isn't reused

style(apollo-language-server): Add return type to removeDirectiveAnno…

6ffd876

…tatedFields This ensures that the type passed in extends from ASTNode and then returns the same type that is passed in, which may be a more specific version of ASTNode (like DocumentNode).

docs: update changelog

f04abf4

justinanastos force-pushed the justin/AP-682/client-check-fragments branch from 83bb034 to f04abf4 Compare July 31, 2019 21:46

jbaxleyiii suggested changes Aug 1, 2019

View reviewed changes

justinanastos requested a review from jbaxleyiii August 2, 2019 16:16

justinanastos commented Aug 2, 2019

View reviewed changes

Comment thread packages/apollo-language-server/src/project/client.ts

justinanastos commented Aug 2, 2019

View reviewed changes

Comment thread packages/apollo-language-server/src/utilities/__tests__/graphql.test.ts Outdated

add more tests, make fragment removal recursive

32601c1

justinanastos force-pushed the justin/AP-682/client-check-fragments branch from fc4b4c9 to 32601c1 Compare August 2, 2019 16:26

jbaxleyiii suggested changes Aug 2, 2019

View reviewed changes

justinanastos added 3 commits August 6, 2019 09:27

comment why we use extends ASTNode

0603b39

remove recursion on removeNodesWithEmptySelectionSets

1628a48

improve comments for recursive orphaned fragments

bfcdbe1

justinanastos requested a review from jbaxleyiii August 6, 2019 13:50

Remove unncessary code to remove fragment spreads

2a60537

We're getting a list of all the fragment spreads and then removing all fragment spreads that aren't in the list we just created. This will never happen.

jbaxleyiii suggested changes Aug 9, 2019

View reviewed changes

justinanastos added 2 commits August 12, 2019 09:48

fix typo in graphql, genetic -> generic

8fc572c

Fix bug in nested visitor and document why we're using it

09161d9

jbaxleyiii approved these changes Aug 12, 2019

View reviewed changes

justinanastos merged commit 724eb1c into master Aug 12, 2019

justinanastos deleted the justin/AP-682/client-check-fragments branch August 12, 2019 21:11

Conversation

justinanastos commented Jul 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jbaxleyiii left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

justinanastos commented Aug 2, 2019

Uh oh!

Uh oh!

Uh oh!

jbaxleyiii left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jbaxleyiii Aug 2, 2019

Choose a reason for hiding this comment

Uh oh!

justinanastos Aug 6, 2019

Choose a reason for hiding this comment

Uh oh!

jbaxleyiii Aug 9, 2019

Choose a reason for hiding this comment

Uh oh!

justinanastos Aug 9, 2019

Choose a reason for hiding this comment

Uh oh!

justinanastos Aug 12, 2019

Choose a reason for hiding this comment

Uh oh!

jbaxleyiii left a comment

Choose a reason for hiding this comment

Uh oh!

jbaxleyiii Aug 9, 2019

Choose a reason for hiding this comment

Uh oh!

jbaxleyiii Aug 9, 2019

Choose a reason for hiding this comment

Uh oh!

jbaxleyiii left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

justinanastos commented Jul 31, 2019 •

edited

Loading