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

[Batching] Query merging seems to discard variables only used in directives #577

Closed
rricard opened this issue Aug 23, 2016 · 5 comments
Closed
Labels

Comments

@rricard
Copy link
Contributor

rricard commented Aug 23, 2016

Similarly to #562, when we activate batching with shouldBatch, variables only used in directives are not injected correctly in the merged query.

For instance, let's say I'm performing the following query:

query SomeQuery($assertion: Boolean!) {
  fieldTrue @include(if: $assertion)
  fieldFalse @skip(if: $assertion)
}

it will be rendered in the batch query the following way:

query ___composed($___SomeQuery___requestIndex_1___assertion: Boolean!) {
  ___SomeQuery___requestIndex_0___fieldIndex_0: fieldTrue @include(if: $assertion)
  ___SomeQuery___requestIndex_0___fieldIndex_1: fieldFalse @skip(if: $assertion)
}

Resulting in the following error:

Variable "$assertion" is not defined by operation "___composed".
@rricard
Copy link
Contributor Author

rricard commented Aug 23, 2016

I'll try to take a look at it, if I find something, let's do a PR!

@rricard
Copy link
Contributor Author

rricard commented Aug 23, 2016

But believe me, once I'm able to activate batching in my codebase, trust me, that means the feature is really stable, I have all of the twisted scenarios you can expect! 😆

@Poincare
Copy link
Contributor

Poincare commented Aug 23, 2016

Yeah, this can be fixed pretty easily by looking into directives within renameVariables inside queries/queryMerging.ts. The number of edge cases on this query merging thing is pretty staggering.

@rricard
Copy link
Contributor Author

rricard commented Aug 23, 2016

@Poincare pretty busy right now but will do when I have time, thanks for the pointers!

rricard added a commit that referenced this issue Aug 24, 2016
Query merging should be able to merge queries with variables in directives correctly:

tests if the variables in a directive are correctly merged.
rricard added a commit that referenced this issue Aug 24, 2016
In query merging, explore directives for varaiable usage and rename effectively the variables.
@rricard
Copy link
Contributor Author

rricard commented Aug 24, 2016

@Poincare patch in #584

rricard added a commit that referenced this issue Aug 25, 2016
Query merging should be able to merge queries with variables in directives correctly:

tests if the variables in a directive are correctly merged.
rricard added a commit that referenced this issue Aug 25, 2016
In query merging, explore directives for varaiable usage and rename effectively the variables.
helfer pushed a commit that referenced this issue Aug 30, 2016
Query merging should be able to merge queries with variables in directives correctly:

tests if the variables in a directive are correctly merged.
helfer pushed a commit that referenced this issue Aug 30, 2016
In query merging, explore directives for varaiable usage and rename effectively the variables.
helfer pushed a commit that referenced this issue Aug 30, 2016
Query merging should be able to merge queries with variables in directives correctly:

tests if the variables in a directive are correctly merged.
helfer pushed a commit that referenced this issue Aug 30, 2016
In query merging, explore directives for varaiable usage and rename effectively the variables.
helfer pushed a commit that referenced this issue Aug 30, 2016
* Add failing test reproducing #577

Query merging should be able to merge queries with variables in directives correctly:

tests if the variables in a directive are correctly merged.

* Fix #577 by exploring directives for renaming

In query merging, explore directives for varaiable usage and rename effectively the variables.

* Add to changelog the changes by #584
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

2 participants