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

docs: variable inlining feature #1013

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

docs: variable inlining feature #1013

wants to merge 1 commit into from

Conversation

obmarg
Copy link
Owner

@obmarg obmarg commented Aug 23, 2024

Why are we making this change?

Cynic now supports inlining variables as a first class feature.

What effects does this change have?

Adds an example and documentation for this feature

TODO:

@obmarg obmarg mentioned this pull request Aug 23, 2024
5 tasks
@obmarg obmarg force-pushed the obmarg/ztmmzmuwolwr branch from 97b5ed5 to bc71a86 Compare August 23, 2024 19:21
Base automatically changed from obmarg/ztmmzmuwolwr to main August 23, 2024 19:24
@Bobrokus
Copy link
Contributor

Why not implement it directly to QueryVariables without having to derive it?

@obmarg
Copy link
Owner Author

obmarg commented Aug 23, 2024

I tried that and it had some downsides:

  1. It added extra trait bound requirements if you're using generic parameters with QueryVariables.
  2. Adding a function to QueryVariables is sort of a breaking change, and I'd rather avoid that (wouldn't break most users, but it's still not ideal)
  3. I suspect the generics that to_input_literal will generate a reasonable amount of serde code for the compiler to process. This feature seems rather niche and I'm not expecting many people to need it so I'd rather only people who opt in pay that price.
  4. I used dynamic dispatch to implement this, so QueryVariableLiterals now needs to be object safe. If I put this functionality on QueryVariables then that would need to be object safe. I'd rather not add that constraint for this niche feature.

@obmarg obmarg force-pushed the obmarg/llmtmlpwkrwz branch from 6a5cdfe to 51ddab8 Compare August 23, 2024 19:38
Copy link

netlify bot commented Aug 23, 2024

Deploy Preview for cynic-querygen-web canceled.

Name Link
🔨 Latest commit 51ddab8
🔍 Latest deploy log https://app.netlify.com/sites/cynic-querygen-web/deploys/66c8e525874f9d00080d0e59

@Bobrokus
Copy link
Contributor

Understand. I'm still really surprised you even took the effort. This would work well with some derive persistence system for query-gen, where you can add additional derives to the generated structs. That would be awesome. I'm sure more people could use this.

@Bobrokus
Copy link
Contributor

Btw why not use generics instead of dyn?

@obmarg
Copy link
Owner Author

obmarg commented Aug 28, 2024

Using generics would have meant one of:

  1. Adding another generic parameter onto almost every defintion in here: https://github.com/obmarg/cynic/blob/main/cynic/src/queries/builders.rs
  • That's a lot of work that I didn't want to do
  • I don't think it gives any real advantage over just using dyn.
  • Adding another generic parameter to SelectionBuilder would mean a breaking change to the QueryFragment trait.
  • Any of the macros that interact with SelectionBuilder would have to become more complicated (possibly in ways that are difficult/impossible to do - although without trying I can't say for sure).
  1. I could have probably used the existing VariablesFields generic param on SelecitonBuilder etc - but then I'd need to add a whole ton of bounds onto all those definitions.
  • This would potentially be another breaking change
  • It'd probably be difficult to opt out of deriving QueryVariableLiterals with that approach
  • Again I just think it'd make things difficult

Whereas dynamic dispatch avoids most of these downsides, and doesn't introduce many I can think of.

@Bobrokus
Copy link
Contributor

Makes sense

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants