jquense
commented
4 years ago @itajaja basically it walks the AST tree and calls every visitor for each node. visitors can change nodes which doesn't mutate the AST but does change what nodes the rest of the visitors see as it traverses the new nodes. The tests only checked the one rule so i missed the obvious problem that this would break other validation rules.
the core concern here is that this now does a full AST traverse for this one rule in addition to the normal single pass done for all other rules. Question is whether an extra AST traversal per request is fine or not. I can't think of a way to implement this during validation that doesn't basically do a second pass. The more "efficient" alternative would be to calculate cost during execution (where the fragments are resolved and merged into one tree) and bail when the cost gets to high, but i don't know that there are hooks for doing that in graphql.js b/c its much worse than other languages in terms of features.
Ok this is annoying. I was hoping to avoid this sort of extra traversal. But I overlooked that obvious changing the ast for one rule is going to affect what other rules see, even if the final AST isn't used for anything. So this was creating invalid tree's that were failing the built in rules. Now this does a full traversal isolated. This was easier than trying to do it the main visit loop and probably not much more inefficient