fix duplicate typescript keys #10531
Conversation
🦋 Changeset detectedLatest commit: 9d89ff6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
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 |
| const outputName = selectionNode.alias.value; | ||
| // Check if a field with the same output name already exists | ||
| // (either as an alias or as a regular field) | ||
| if (!primitiveFields.has(outputName)) { |
There was a problem hiding this comment.
the two maps primitiveFields and primitiveAlias fields are the root cause of the problem here i've updated them so they are now aware of each other which fixes the bug
… into fix-duplicate-ts-keys
| id: realId | ||
| name |
There was a problem hiding this comment.
I think this'd result in a GraphQL Validation error, so I wonder if we need to cater for it?
If my test setup is wrong, let me know how to fix it 🙂
There was a problem hiding this comment.
let me try out schema and double check if you're right and our code is just letting us do somethig invalid, its either that or my reproduction case isn't quite right. Hopefully you can see from the code itself there might be a problem though where variable names get duplicated, i'll work on the repro case
There was a problem hiding this comment.
Yeah, i just looked out the output of our deprecated Apollo codegen and frustratingly, they let us get away with this
with a warning but fixed the types silently
I guess this likely would with this library:
- have no warning
- break the types (not so silently but not obviously)
both aren't ideal, is there a way to get the warnings from validation for the presets?
There was a problem hiding this comment.
I have skip validation config setting on, that makes sense why this isn't breaking - guess the question is if this library should only generate valid typescript when validation is enabled, or should try to ensure it always generates valid code?
There was a problem hiding this comment.
interestingly, i cannot get a validation error for this specific error in this tooling, it does not detect it:
- old tooling detects it
- graphiql detect it
- codegen tooling doesn't pick up the validation error
There was a problem hiding this comment.
@eddeee888 That's odd, try using the file operation preset from community maybe it hits a different path than client preset or the typescript one
Failing that I can try to refine the repro case it could be I broke it when I copy pasted and anonymised it from our internal schema
There was a problem hiding this comment.
I've set that up here: eddeee888/graphql-server-template#142, but it's still not reproducible 🙂
There was a problem hiding this comment.
@eddeee888 WEIRD, I also cannot reproduce it now, I'm very confused, apologies for the time wasting, if I can somehow try to get back to where I was with the issue, I'll let you know, but can't anymore. I was pretty sure i had a reliable replication case but i can't get it to happen now.
I guess this just leaves the question of the original intent of this PR:
- You can enable
skipDocumentsValidation: true. It seems this is at odds with the bug in this PR, where it's possible to generate invalid typescript code. If the CLI is only intended to work with valid queries, then I'd question the purpose ofskipDocumentsValidation.
Happy either way though, let me know and i can close if need be
There was a problem hiding this comment.
Thanks for helping me confirm the reprod repo @Georgegriff !
Regarding skipDocumentsValidation, I agree it's an odd config 😅 I'll take a look in the future to see if we could drop it.
There was a problem hiding this comment.
I'll close this for now 🙂
2 participants
🚨 IMPORTANT: Please do not create a Pull Request without creating an issue first.
Any change needs to be discussed before proceeding. Failure to do so may result in the rejection of the pull request.
Description
See issue #10532 for a description
The problem here is that two maps are used for tracking keys
primitiveAliasFieldsandprimitiveFieldsso they we'ren't aware if you would end up with duplicate keys.The simplest fix seems to be to just ensure we don't add duplicate fields to the type, since they should always be the same ts type here (i think?) it should be fine... i hope, this does seem like an edge case.. although one we've hit in our production code
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
Related # #10532
Type of change
Please delete options that are not relevant.
Screenshots/Sandbox (if appropriate/relevant):
Adding links to sandbox or providing screenshots can help us understand more about this PR and take action on it as appropriate
How Has This Been Tested?
Unit tests
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Test Environment:
@graphql-codegen/...:Checklist:
Further comments
If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...