-
Notifications
You must be signed in to change notification settings - Fork 447
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(graphql): add custom logic for reference unions #8350
fix(graphql): add custom logic for reference unions #8350
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
2 Skipped Deployments
|
|
||
return { | ||
type: name, | ||
// references: targetTypes, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't want to have this comment 😅
Would we like to have this? It seems like what we would like to do, based on the usage in getUnionDefinition, but this implementation adds quite a bit of content to our testing snapshots.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you make it "optional"?
const def = {type: name, ...base}
if (targetTypes !== undefined) {
def.references = targetTypes
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, added now! I realize my previous hesitation wasn't clear -- it seems like we weren't effectively adding this array in getUnionDefinition
(although it seems we intended to), so I was worried about adding the 7k extra lines in our snapshots that you see now. I don't know the code well enough to know if this is duplicative, if there are side effects, etc.
No changes to documentation |
Component Testing Report Updated Jan 23, 2025 2:58 PM (UTC) ❌ Failed Tests (2) -- expand for details
|
⚡️ Editor Performance ReportUpdated Thu, 23 Jan 2025 15:00:21 GMT
Detailed information🏠 Reference resultThe performance result of
🧪 Experiment resultThe performance result of this branch
📚 Glossary
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems reasonable based on what I learned while doing that recent PR that this improves upon. However, I'm not an expert in this code and haven't looked at the typegen code so I think it's best if I defer to Sindre on this one.
|
||
return { | ||
type: name, | ||
// references: targetTypes, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you make it "optional"?
const def = {type: name, ...base}
if (targetTypes !== undefined) {
def.references = targetTypes
}
2307ce3
to
8f67dcf
Compare
The merge-base changed after approval.
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
8f67dcf
to
0ec698d
Compare
0ec698d
to
c58eb1b
Compare
Description
We discovered a bug where a union-style reference definition that referenced itself (e.g.:
would not correctly bail out of the
getUnionDefinition
recursion and cause amaximum calls exceeded
error. This PR better aligns our reference definition logic to what we do with type generation by adding a few lines to code to thegetReferenceDefinition
function and avoiding the union definition logic altogether.What to review
references
array? I've commented it out for now just because it would make our new snapshots a bit noisy. We've done without this array for quite a while, but it seems nice to have.Testing
Please review the updated snapshots and make sure all is well.
Notes for release
Fixes an issue where referencing a type of the same kind of document within a document would cause a
Maximum call stack size exceeded
error during GraphQL schema generation.