chore: introduce query planner to router-bridge #1090
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
o0Ignition0o
force-pushed
the
igni/router_bridge_query_planner
branch
from
4b831ba
to
9782a5f
Compare
o0Ignition0o
changed the title
o0Ignition0o
force-pushed
the
igni/router_bridge_query_planner
branch
from
9782a5f
to
a843edd
Compare
o0Ignition0o
requested review from
abernix,
trevor-scheer,
BrynCooke and
EverlastingBugstopper
BrynCooke
approved these changes
Oct 15, 2021
abernix
requested changes
Oct 15, 2021
Comment on lines
-1
to
-4
| { | ||
| "extends": "../tsconfig.test.base", | ||
| "include": ["**/__tests__/**/*"], | ||
| "references": [ |
There was a problem hiding this comment.
I'm slightly unsure that we should remove this Jest configuration from this repository. Most users of this repo are not running cargo test, but instead working in TypeScript. With this removed, they might not understand a change that they made is breaking until after it goes through CI. Could we avoid that by running something basic that we need (like a simple query plan, possibly copied from existing query plan tests?)
There was a problem hiding this comment.
mind pairing with me on that? I struggled a lot yesterday because whatever i tried to do the jest tests declared in the bridge wouldn't run :(
There was a problem hiding this comment.
On second thought, you're right, if the graphql api or the query-planner-js api changes, we might warn users that we can't run the bridge as intended
BrynCooke
approved these changes
Oct 15, 2021
o0Ignition0o
force-pushed
the
igni/router_bridge_query_planner
branch
2 times, most recently
from
bf3c26b
to
1be695b
Compare
fixes #1088 This commit is built on top of @BrynCooke 's work on #812, and exposes a query planning functionality, which has been living in a separate branch for a while. A few things happened: General: - Remove jest tests from the router bridge (they weren't being run anyway, and they are now superseded by the cargo tests) - Add router-bridge js files to the prettier configuration for formatting and linting - Add a dependency to query-planner-js to the router-bridge TypeScript/Javascript: - Add `do_plan.ts` that calls the query-planner behind the scenes. - Use `new QueryPlanner(schema).buildQueryPlan` instead of the old `buildQueryPlan` function, that doesn't exist anymore (depending on how long we want to keep this code, we might be able to keep the QueryPlanner object around, and skip subsequent schema parsing / composing. - Move the type `OperationResult` to a utils file (so we can use it in both `do_plan.ts` and `do_introspect.ts` Rust: - Use &str instead of String for schema SDL, since apollo-rs provides as_str() on the Schema type. - Add a plan function, that is generic over it's return type ( `Result<T, PlanningErrors>` where `T: DeserializeOwned + 'static` so we can keep the `QueryPlan` structure on the router-core side. - Add plan() tests that call plan<serde_json::Value>() so we have snapshots over the plan result.
o0Ignition0o
force-pushed
the
igni/router_bridge_query_planner
branch
from
1be695b
to
0e86671
Compare
…ght that we didn't validate queries, and add a cargo test
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
chore: Introduce query planner to router-bridge
fixes #1088
#1079 Added a new router-bridge that binds together the rust router and some TypeScript / Javascript capabilities (introspection mostly).
This commit is built on top of @BrynCooke 's work on #812, and exposes a query planning functionality, which has been living in a separate branch for a while.
A few things happened:
General:
TypeScript/Javascript:
do_plan.tsthat calls the query-planner behind the scenes.new QueryPlanner(schema).buildQueryPlaninstead of the oldbuildQueryPlanfunction, that doesn't exist anymore (depending on how long we want to keep this code, we might be able to keep the QueryPlanner object around, and skip subsequent schema parsing / composing.OperationResultto a utils file (so we can use it in bothdo_plan.tsanddo_introspect.tsRust:
Result<T, PlanningErrors>whereT: DeserializeOwned + 'staticso we can keep theQueryPlanstructure on the router-core side.