Repository navigation
Visitor trait? #25
Description
Activity
Well, i definitely want some kind of visitor in the library. But I'm not sure which one is good. Usually, I like iterator-style visitors better than what's linked (obviously it's not always feasible too). Also, mutating visitor makes a lot of sense.
I think I would wait for several other projects that can share the same visitor type before upstreaming. But I'll be glad to listen to what others think about that (i.e. is there something that would be useful both for juniper itself and juniper-from-schema?)
Totally. I haven’t found a one best way to build visitors yet, but the PR that I’ve submitted is inspired by the syn crate and what I’ve found myself to be most useful.
I don’t quite know what you mean by “iterator-style” visitor however. Do you have a link to an example?
Bringing in @theduke and @tomhoule who might have opinions here 😊
Sorry for the very late reply! I didn't have a strong opinion spontaneously the first time I read this, so I didn't reply, but now I think it would make sense to use it in graphql-client. I've been thinking about refactoring the code generation step and I think using something like the query visitor in your PR would be cleaner/easier to understand than the explicit recursion.
@tailhook I have a similar visitor trait in juniper-from-schema. Here are some example of how I'm using it
- Walking the AST once to gather data needed to answer questions like "is the type
Fooa scalar or a type?". This is necessary for code generation. https://github.com/davidpdrsn/juniper-from-schema/blob/master/juniper-from-schema-code-gen/src/ast_pass/ast_data_pass.rs - Final code generation. https://github.com/davidpdrsn/juniper-from-schema/blob/master/juniper-from-schema-code-gen/src/ast_pass/code_gen_pass.rs
- Walking the AST once to gather data needed to answer questions like "is the type
Walking the AST once to gather data needed to answer questions like "is the type Foo a scalar or a type?
Isn't it just walk over top-level definitions? I mean plain simple loop. It is much cheaper than full-blown recursive visitor. And graphql schema structure is simple enough.
Final code generation.
Well, I'm not sure that visitor like in #26 is helpful here. I.e. to generate things for recursive fields you often need something like:
trait Visitor { fn enter_field(...); fn exit_field(...); }Rather than just:
trait Visitor { fn visit_field(...); }Also I find flat visitor where you have all kinds of things:
fn enter_document() fn enter_query() fn enter_mutation() fn enter_field() fn visit_variable_definition()very unsatisfactory in such situation, i.e. you may want to pass some information from
enter_querytovisit_variable_definition. And you has to put it on the visitor instance into someOption, thenunwrap()it in the inner visitor andtake()it in theleave_function. This is quite fragile code structure comparing to just plain rust code:fn codegen_query(q: &Query) -> Result<..> { // emit beginning for var in &q.variables { codegen_variable(var, some_extra_data, q)?; } }(this is how formatting code is structured for example)
Also visitor is easier, if you emitting something to a buffer as a side-effect. Emitting AST via visitor is much more complex.
So I'm still not sure at all.
Follow up: I think simple visitors that map enum variant to method call are just legacy from the languages that don't have pattern-matching. I.e. this is a way to emulate pattern-matching in those languages. But we don't need that in Rust.
I don’t quite know what you mean by “iterator-style” visitor however. Do you have a link to an example?
I mean a visitor where you can iterate over nodes rather than having callbacks:
for item in visit(&document) { match item { Field(f) => fields += 1, Query(q) => queries += 1, } } println!("Document has {} fields and {} queries in total", fields, queries).This may be useful for very simple things, like statistics or finding all type names used.
I've made a sketch of "iterative visitor" in #31, which potentially allows iterating over different levels of granularity:
let mut field_names = Vec::new(); for f in doc.visit::<Field<_>>() { fields += 1; field_names.push(f.name); }
The implementation is a bit complex with types (and their names should be changed, probably), and is a bit verbose (each type pair has to have some code). But is pretty straightforward, as much as flat iterator over a hierarchical data structure can be (see this commit as an example). And perhaps may be reduced by using more generics or few macros.
I'm not sure I'll have time to finish whole implementation soon. But we can merge in some functional part of it if soon. There are not so many truly recursive structures in the GraphQL. For other things it adds much less value.
What do you think?
Reacted by Vlady VeselinovI'm interested in contributing ❤️ It seems that every Rust graphql package currently uses their own visitor pattern. I believe adding this functionality to graphql-parser will make custom validation easier for many people who don't have the time to write their own implementation from scratch.
Long story short, how can I help?
Further notes:
- I see Add SchemaVisitor and QueryVisitor traits and functions #26 got stalled, maybe we can revive that PR?
- My Twitter thread asking who's interested
Crates that have their own private implementation:
You see the pattern here, if we do this in graphql-parser, everyone will be able to parse and validate queries without writing their own implementation.
For inspiration: Graphql-js has quite a rich set of functionality for this, I wonder if we can get inspired from things like
Reacted by Veeti Haapsamo, Shay Cichocki, Rikki Schulte and Dotan SimhaThis may be useful for very simple things, like statistics or finding all type names used.
I wonder if we can support validation and modification with this pattern. Maybe we can add a
&mut selfto the signature to allow people to modify the AST and report errors? Here's how Relay does it for example.Reacted by Rikki Schulte and Dotan SimhaThis may be useful for very simple things, like statistics or finding all type names used.
I wonder if we can support validation and modification with this pattern. Maybe we can add a
&mut selfto the signature to allow people to modify the AST and report errors? Here's how Relay does it for example.We implemented a visitor (with type info) in https://github.com/dotansimha/graphql-tools-rs , and also a (almost) spec-compliant validation phase. Feedback is welcome ;)
Reacted by Paul Colomiets
As part of juniper-from-schema I have made a visitor trait that handles walking over the schema AST. I'm thinking if it makes sense to move that trait upstream to here so others can use it as well. I would also make one for query documents ofc.
If you think thats a good idea I'll cook up a PR.