-
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
/* eslint-disable promise/always-return */ | ||
import { runAuthenticatedQuery } from "schema/v2/test/utils" | ||
|
||
describe("recording an artwork view", () => { | ||
const query = ` | ||
mutation { | ||
recordArtworkView(input: { artwork_id: "artwork-id" }) { | ||
artworkId | ||
} | ||
} | ||
` | ||
|
||
const context = { | ||
createArtworkViewLoader: (id) => Promise.resolve({ artwork_id: id }), | ||
} | ||
|
||
it("records an artwork view", async () => { | ||
const data = await runAuthenticatedQuery(query, context) | ||
expect(data).toEqual({ | ||
recordArtworkView: { | ||
artworkId: "artwork-id", | ||
}, | ||
}) | ||
}) | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
/* eslint-disable promise/always-return */ | ||
import { runAuthenticatedQuery } from "schema/v2/test/utils" | ||
|
||
describe("removing an artwork view", () => { | ||
const query = ` | ||
mutation { | ||
removeArtworkView(input: { artwork_id: "artwork-id" }) { | ||
artworkId | ||
} | ||
} | ||
` | ||
|
||
const context = { | ||
deleteArtworkViewLoader: (id) => Promise.resolve({ artwork_id: id }), | ||
} | ||
|
||
it("removes an artwork view", async () => { | ||
const data = await runAuthenticatedQuery(query, context) | ||
expect(data).toEqual({ | ||
removeArtworkView: { | ||
artworkId: "artwork-id", | ||
}, | ||
}) | ||
}) | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,43 @@ | ||
import { GraphQLNonNull, GraphQLString } from "graphql" | ||
import { mutationWithClientMutationId } from "graphql-relay" | ||
import { ResolverContext } from "types/graphql" | ||
import { GraphQLError } from "graphql" | ||
|
||
export interface RecordArtworkViewMutationInput { | ||
artwork_id: string | ||
} | ||
|
||
export const recordArtworkViewMutation = mutationWithClientMutationId< | ||
RecordArtworkViewMutationInput, | ||
any, | ||
ResolverContext | ||
>({ | ||
name: "RecordArtworkViewMutation", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wondering, if you omit There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
description: "Record an artwork view.", | ||
inputFields: { | ||
artwork_id: { | ||
type: new GraphQLNonNull(GraphQLString), | ||
description: "ID of artwork.", | ||
}, | ||
}, | ||
outputFields: { | ||
artwork_id: { | ||
description: "ID of viewed artwork.", | ||
type: new GraphQLNonNull(GraphQLString), | ||
deprecationReason: "Use artworkId.", | ||
}, | ||
artworkId: { | ||
description: "ID of viewed artwork.", | ||
type: new GraphQLNonNull(GraphQLString), | ||
}, | ||
}, | ||
mutateAndGetPayload: async ({ artwork_id }, { createArtworkViewLoader }) => { | ||
try { | ||
const response = await createArtworkViewLoader(artwork_id) | ||
|
||
return { artwork_id: response.artwork_id, artworkId: response.artwork_id } | ||
} catch (error) { | ||
throw new GraphQLError(`RecordArtworkViewMutation error: ${error}`) | ||
} | ||
}, | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,43 @@ | ||
import { GraphQLNonNull, GraphQLString } from "graphql" | ||
import { mutationWithClientMutationId } from "graphql-relay" | ||
import { ResolverContext } from "types/graphql" | ||
import { GraphQLError } from "graphql" | ||
|
||
export interface RemoveArtworkViewMutationInput { | ||
artwork_id: string | ||
} | ||
|
||
export const removeArtworkViewMutation = mutationWithClientMutationId< | ||
RemoveArtworkViewMutationInput, | ||
any, | ||
ResolverContext | ||
>({ | ||
name: "RemoveArtworkViewMutation", | ||
description: "Remove an artwork view.", | ||
inputFields: { | ||
artwork_id: { | ||
type: new GraphQLNonNull(GraphQLString), | ||
description: "ID of artwork.", | ||
}, | ||
}, | ||
outputFields: { | ||
artwork_id: { | ||
description: "ID of viewed artwork.", | ||
type: new GraphQLNonNull(GraphQLString), | ||
deprecationReason: "Use artworkId.", | ||
}, | ||
artworkId: { | ||
description: "ID of viewed artwork.", | ||
type: new GraphQLNonNull(GraphQLString), | ||
}, | ||
}, | ||
mutateAndGetPayload: async ({ artwork_id }, { deleteArtworkViewLoader }) => { | ||
try { | ||
const response = await deleteArtworkViewLoader(artwork_id) | ||
|
||
return { artworkId: response.artwork_id } | ||
} catch (error) { | ||
throw new GraphQLError(`RemoveArtworkViewMutation error: ${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.
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: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).