Progressive @override (composition only)
#2879
Conversation
|
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
trevor-scheer
force-pushed
the
trevor/progressive-override-composition
branch
from
de91f3a to
4587eaf
Compare
trevor-scheer
force-pushed
the
trevor/progressive-override-composition
branch
2 times, most recently
from
1f4badd to
6357424
Compare
trevor-scheer
force-pushed
the
trevor/progressive-override-composition
branch
from
6357424 to
1b913e7
Compare
| .add(new JoinSpecDefinition(new FeatureVersion(0, 3), new FeatureVersion(2, 0))) | ||
| .add(new JoinSpecDefinition(new FeatureVersion(0, 4), new FeatureVersion(2, 7))); |
There was a problem hiding this comment.
My understanding of the effect of these lines is that:
- If the latest federation version seen in composition is between v2.0 and v2.6, then join spec v0.3 will be used.
- If the latest federation version seen in composition is v2.7 or higher, then join spec v0.4 will be used.
From #2528 it looks like we want to use the earliest version of join spec permissible (for new versions at least), to avoid immediately breaking gateways/routers if people upgrade composition and forget to upgrade gateway/router first.
Which is mostly fine, but it probably deserves a note in the changelog, since it means customers who want to test a new version of composition will need to additionally bump at least one subgraph to v2.7 if they want to really test it (otherwise the first subgraph team trying to using v2.7 may encounter the issues instead of the team asked to test compatibility).
| @@ -1248,6 +1270,12 @@ class Merger { | |||
| overriddenSubgraphASTNode, | |||
| )); | |||
| } | |||
|
|
|||
| // capture an override label if it exists | |||
There was a problem hiding this comment.
We might want to do some kind of validation on the label here (it's easier to relax validations in the future than to add them). For example, we could require that the label must either (1) start with a letter and consist of alphanumeric+dashes+underscore(+periods+slashes+colons if we feel like it), or (2) be of the form percent(x) where x is a number between 0 and 100 inclusive (maybe something like BigNumber from bignumber.js could help here). If you have any other syntax in mind for label we could use that too.
There was a problem hiding this comment.
Good call out. I don't think we need a lib for this but let me know if you think I've missed anything in how we handle floats.
cf27bcb
There was a problem hiding this comment.
It was mainly to handle cases where JS Numbers lose precision and we don't faithfully copy the argument from subgraphs to supergraph, but if you're using 10 digits total (1-2 before the decimal point and 8 after), then JS Numbers should be able to faithfully represent that.
There was a problem hiding this comment.
Cool. I think capping at 8 decimal points makes for a reasonable usage requirement that's also simple to enforce and communicate on our end, so if you don't see any other issue here I'll keep as is.
trevor-scheer
force-pushed
the
trevor/progressive-override-composition
branch
from
1ab7c53 to
cf27bcb
Compare
There was a problem hiding this comment.
Just some minor things below. (You'll also want to update the changeset regarding testing strategy, as noted here.)
| @@ -1248,6 +1270,12 @@ class Merger { | |||
| overriddenSubgraphASTNode, | |||
| )); | |||
| } | |||
|
|
|||
| // capture an override label if it exists | |||
There was a problem hiding this comment.
It was mainly to handle cases where JS Numbers lose precision and we don't faithfully copy the argument from subgraphs to supergraph, but if you're using 10 digits total (1-2 before the decimal point and 8 after), then JS Numbers should be able to faithfully represent that.
trevor-scheer
merged this pull request
into
trevor/progressive-override
Add new optional `label` arg to `@override` which is a `String`. Capture label in the supergraph via the new `@join__field` arg `overrideLabel` so these values can be used during query graph creation and query planning.
Add new optional `label` arg to `@override` which is a `String`. Capture label in the supergraph via the new `@join__field` arg `overrideLabel` so these values can be used during query graph creation and query planning.
Add new optional `label` arg to `@override` which is a `String`. Capture label in the supergraph via the new `@join__field` arg `overrideLabel` so these values can be used during query graph creation and query planning. Reviewed in two separate PRs: #2879 #2902 --------- Co-authored-by: Sachin D. Shinde <[email protected]>
Bump fed spec to v2.7
Bump join spec to v0.4
Add new optional
labelarg to@overridewhich is aString.Capture label in the supergraph via the new
@join__fieldargoverrideLabelso these values can be used during query graph creation and query planning.