-
-
Notifications
You must be signed in to change notification settings - Fork 531
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: Remove GlobalID type from relay schema. #3180
base: main
Are you sure you want to change the base?
fix: Remove GlobalID type from relay schema. #3180
Conversation
Hi, thanks for contributing to Strawberry 🍓! We noticed that this PR is missing a So as soon as this PR is merged, a release will be made 🚀. Here's an example of Release type: patch
Description of the changes, ideally with some examples, if adding a new feature. Release type can be one of patch, minor or major. We use semver, so make sure to pick the appropriate type. If in doubt feel free to ask :) Here's the tweet text:
|
1 similar comment
Hi, thanks for contributing to Strawberry 🍓! We noticed that this PR is missing a So as soon as this PR is merged, a release will be made 🚀. Here's an example of Release type: patch
Description of the changes, ideally with some examples, if adding a new feature. Release type can be one of patch, minor or major. We use semver, so make sure to pick the appropriate type. If in doubt feel free to ask :) Here's the tweet text:
|
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #3180 +/- ##
=======================================
Coverage 96.53% 96.53%
=======================================
Files 484 484
Lines 29812 29814 +2
Branches 3680 3680
=======================================
+ Hits 28780 28782 +2
Misses 849 849
Partials 183 183 |
CodSpeed Performance ReportMerging #3180 will not alter performanceComparing Summary
|
Having this type in the schema violates the relay spec. The GlobalID is technically an interpretation of the ID type, and the schema should always return ID for that. I created a new scalar type called ID. It's unclear how this collides with the strawberry.ID, but given that strawberry.ID isn't actually a scalar type, I am hoping it is ok.
3a4213e
to
63594fe
Compare
for more information, see https://pre-commit.ci
This is something that a lot of users already asked for: a way to have the scalar named There're just 2 issues that I see with this PR:
|
Could we make it configurable somehow? It might be painful to maintain though, but at least it gives people more time to upgrade
We could throw an error? or make relay.ID an alias? |
users could do the reverse of this comment (#3551 (comment)) to rename relay.ID back to GloabalID until they find time to properly migrate.
I think we should. Using both scalars is violating the relay specs anyways |
+1. I think this should be merged soon! :) |
I think a file inside Mostly, we need to make sure that the users know that:
Also add an example on how to expose it as |
Description
Having this type in the schema violates the relay spec:
https://graphql.org/learn/global-object-identification/
https://relay.dev/docs/guides/graphql-server-specification/#schema
The GlobalID is technically a specialization of the existing strawberry.ID type, and the schema name should be "ID". However, we still want the ability to extract the type and internal ID from the GlobalID.
I don't really think this is the "correct" solution, but I wanted to put it up in case it helps with the discussion I started here: #3177
I am open to discussion/suggestion if this is "close" to how @patrick91 wants to do it; otherwise happy to close it without merging once your more correct solution comes out.
This is a breaking change, as anyone who is relying on GlobalID in their relay schema is going to be affected, especially on client code.
The lint error that arises is exactly what I was afraid of and I'm not clear how to resolve it.
Types of Changes
Issues Fixed or Closed by This PR
Checklist