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

fix(federation): merge the elements if the shared root field is a list #6238

Merged
merged 2 commits into from
Jun 5, 2024

Conversation

ardatan
Copy link
Owner

@ardatan ardatan commented Jun 4, 2024

Merge the elements of the lists if the root field is shared across different subgraphs

type Query {
  products: [Product] # If this field is returned by multiple subgraphs, the elements of the lists will be merged
}

Copy link

changeset-bot bot commented Jun 4, 2024

🦋 Changeset detected

Latest commit: 3e5ddc0

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@graphql-tools/federation Patch
@graphql-tools/utils Patch
federation-benchmark Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

github-actions bot commented Jun 4, 2024

🚀 Snapshot Release (alpha)

The latest changes of this PR are available as alpha on npm (based on the declared changesets):

Package Version Info
@graphql-tools/federation 2.0.1-alpha-20240604141848-3e5ddc0bed3db3f0d65611cf4de65e9f38fcb7dd npm ↗︎ unpkg ↗︎
@graphql-tools/utils 10.2.2-alpha-20240604141848-3e5ddc0bed3db3f0d65611cf4de65e9f38fcb7dd npm ↗︎ unpkg ↗︎

Copy link
Contributor

github-actions bot commented Jun 4, 2024

✅ Benchmark Results

     ✓ no_errors
     ✓ expected_result

     checks.........................: 100.00% ✓ 304       ✗ 0  
     data_received..................: 35 MB   3.5 MB/s
     data_sent......................: 130 kB  13 kB/s
     http_req_blocked...............: avg=4.47µs   min=2.14µs   med=2.9µs    max=195.03µs p(90)=4.59µs   p(95)=5.01µs  
     http_req_connecting............: avg=872ns    min=0s       med=0s       max=132.65µs p(90)=0s       p(95)=0s      
     http_req_duration..............: avg=61.14ms  min=52.78ms  med=58.66ms  max=160.28ms p(90)=66.19ms  p(95)=80.22ms 
       { expected_response:true }...: avg=61.14ms  min=52.78ms  med=58.66ms  max=160.28ms p(90)=66.19ms  p(95)=80.22ms 
     http_req_failed................: 0.00%   ✓ 0         ✗ 152
     http_req_receiving.............: avg=140.15µs min=108.94µs med=134.67µs max=370.42µs p(90)=156.79µs p(95)=167.38µs
     http_req_sending...............: avg=61.71µs  min=20.18µs  med=25.61µs  max=5.39ms   p(90)=33.47µs  p(95)=35.71µs 
     http_req_tls_handshaking.......: avg=0s       min=0s       med=0s       max=0s       p(90)=0s       p(95)=0s      
     http_req_waiting...............: avg=60.94ms  min=52.62ms  med=58.48ms  max=159.88ms p(90)=65.03ms  p(95)=80.05ms 
     http_reqs......................: 152     15.182942/s
     iteration_duration.............: avg=65.84ms  min=56.29ms  med=62.91ms  max=164.61ms p(90)=74.54ms  p(95)=84.77ms 
     iterations.....................: 152     15.182942/s
     vus............................: 1       min=1       max=1
     vus_max........................: 1       min=1       max=1

Copy link
Contributor

github-actions bot commented Jun 4, 2024

💻 Website Preview

The latest changes are available as preview in: https://075df62f.graphql-tools.pages.dev

const allArrays = sources.every(
source => Array.isArray(source) && source.length === target.length,
);
if (allArrays) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't do anything if they don't all have the same length ?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I put it behind the flag. So it depends. If they are arrays but not in the same length or respectArrayLength flag is disabled, the array is concatenated.

@ardatan ardatan requested a review from EmrysMyrddin June 4, 2024 15:06
});

if (areArraysInTheSameLength) {
return new Array(expectedLength).fill(null).map((_, index) =>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why creating a new Array ? We could just map over the first source is they have all the same length ? :-)

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TypeScript fun :D When I do sources[0], it doesn't see it as an array so I have to cast it

@ardatan ardatan merged commit 0f7059b into master Jun 5, 2024
30 checks passed
@ardatan ardatan deleted the handle-shared-root-lists branch June 5, 2024 09:05
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