-
Notifications
You must be signed in to change notification settings - Fork 89
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
feat: add (record|remove)ArtworkView mutations #6323
base: main
Are you sure you want to change the base?
Conversation
|
- add recordArtworkView mutation to replace stitched version from Gravity - new removeArtworkView mutation
edb0489
to
8680810
Compare
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.
Cool! Left a small suggestion how to avoid breaking relay change
@@ -17717,23 +17723,18 @@ type RecentlySoldArtworkTypeEdge { | |||
node: RecentlySoldArtworkType | |||
} | |||
|
|||
# Autogenerated input type of RecordArtworkView | |||
input RecordArtworkViewInput { |
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.
Did a cool thingy @mzikherman taught me - if not sure whether schema will add a breaking change - copy new schema to clients and run yarn relay
. I did it in Force and Eigen. Force doesn't complain, but Eigen does:
[ERROR] Error: ✖︎ Unknown type 'RecordArtworkViewInput'. Did you mean `RecordArtworkViewMutationInput`?
src/app/Scenes/Artwork/Artwork.tsx:261:59
260 │
261 │ mutation ArtworkMarkAsRecentlyViewedQuery($input: RecordArtworkViewInput!) {
│ ^^^^^^^^^^^^^^^^^^^^^^
262 │ recordArtworkView(input: $input) {
[ERROR] Compilation failed.
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.
Thanks!! Yeah, this will cause a compilation error for sure because Eigen is explicitly referencing RecordArtworkViewInput
which has been removed in this diff. I wonder though if this would also cause a runtime error? Will check on that... if this doesn't cause any runtime backwards-incompatibility, I think we could merge this in (with type name changes) and follow-up with an Eigen PR to address the compilation error.
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.
Don't quote me on this - but I think changing the name like that, if a Relay client explicitly references the name - will indeed cause a runtime error if you merge the schema update. Give it a whirl to confirm (and we start to increase our mental model of how to think about these kinds of changes with our clients).
any, | ||
ResolverContext | ||
>({ | ||
name: "RecordArtworkViewMutation", |
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.
Wondering, if you omit Mutation
here - will it produce the schema without changes that break relay (RecordArtworkViewInput
-> RecordArtworkViewMutationInput
)
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.
Hmmm, now that I look closer it seems like we're a bit inconsistent with suffixing these names with Mutation
in the file itself... I wonder if relay adds Mutation anyway but I'll check.
Leverages API endpoints added in https://github.com/artsy/gravity/pull/18432.