Skip to content
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

Update operation docs #1802

Open
wants to merge 7 commits into
base: source
Choose a base branch
from

Conversation

mandiwise
Copy link
Contributor

Addresses #250, #584, #728, #754, and #992

Description

This PR contains updated operations-related content in the Learn docs. Key changes include:

  • Expanded content on GraphQL query operations
  • A separate "Mutations" page with additional examples for updating and deleting data
  • A new "Subscriptions" page
  • Updates to the Star Wars schema to support the new mutation examples
  • Operation-related content now comes after the Schemas and Types page in the left nav
  • Standardization of capitalization and naming for named types

@benjie @jorydotcom

Copy link

vercel bot commented Oct 26, 2024

@mandiwise is attempting to deploy a commit to the The GraphQL Foundation Team on Vercel.

A member of the Team first needs to authorize it.

Copy link

vercel bot commented Oct 27, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
graphql-github-io ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 18, 2024 5:34pm

Copy link
Contributor

@Urigo Urigo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so excited for this!


## Deleting data

Just as we can send a `DELETE` request to delete a resource with a REST API, we can use mutations to delete some existing data as well by defining another field on the `Mutation` type:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should clarify that in GraphQL we send a POST request to delete

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have an updated "Serving over HTTP" page almost ready to go where I go into this in detail based on the draft GraphQL over HTTP spec (including when GET may be used too). Does it make sense to duplicate that here? My goal was draw connections with REST patterns the reader would likely be familiar with while explaining how different kinds of mutations work without overloading with too much information. It can be tricky to find the right balance though! Let me know what you think.

Copy link
Member

@benjie benjie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love the improvements to the queries page (and include a number of minor suggested edits), but I have concerns over the mutations page. I have not yet read the subscriptions page.

src/pages/learn/queries.mdx Outdated Show resolved Hide resolved
src/pages/learn/queries.mdx Outdated Show resolved Hide resolved
src/pages/learn/queries.mdx Show resolved Hide resolved
src/pages/learn/queries.mdx Outdated Show resolved Hide resolved
src/pages/learn/queries.mdx Outdated Show resolved Hide resolved
src/pages/learn/queries.mdx Outdated Show resolved Hide resolved
src/pages/learn/queries.mdx Outdated Show resolved Hide resolved
src/pages/learn/queries.mdx Outdated Show resolved Hide resolved
src/pages/learn/mutations.mdx Outdated Show resolved Hide resolved

In REST, any request might cause some side-effects on the server, but by convention, it's suggested that one doesn't use `GET` requests to modify data. GraphQL is similar—technically any query could be implemented to cause a data write. However, it's useful to establish a convention that any operations that cause writes should be sent explicitly via a mutation.

On this page, you'll learn how to use mutation operations to create, update, and delete data using GraphQL.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am very uncomfortable with this positioning; GraphQL isn't just a CRUD system and in general it's better to use semantically named mutations for specific user actions rather than trying to fit every mutation into the CRUD box. Some mutations such as processPayment, resetPassword, sendEmailVerification, inviteUser, markAllNotificationsAsRead, redeemGiftCard and so on (arguably) don't even fit into the CRUD box. (This isn't to say that CRUD should never be used, but more that a semantically named action should be exposed preferentially where it makes sense to do so.)

From GraphQL's point of view there are no different types of mutation - there is no difference between a create, update, delete or arbitrary user action mutation - so I'm concerned that highlighting CRUD mutations in this way sets the reader (especially someone who does not read the page fully) up for misconceptions.

Copy link
Contributor Author

@mandiwise mandiwise Nov 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see where you're coming from. I think the challenge here is to figure out how much information to include up-front in the intro for a reader who has some experience with REST and needs to connect those dots with GraphQL (e.g. #992). This distinction is called out in the paragraph that follows the new example for updating a human name, but it's halfway down the page.

Do you think it would be better to generalize it like this?

On this page, you'll learn how to use mutation operations to modify data on a server using GraphQL.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think the subheads on the page are problematic? We could further deemphasize any kind of 1:1 CRUD implication by renaming them like this:

Create data Add new data
Update data Update existing data
Delete data Remove existing data

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more thought on this is—I think there's a gap in the Best Practices content where "migrating from REST" could be covered in more detail too with a few more detailed examples that are based on a further-enhanced version of the Star Wars schema.

Copy link
Member

@benjie benjie Nov 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure that our learn docs should necessarily cater primarily to people who already know REST/CRUD - perhaps a separate guide for "how to convert CRUD REST operations to GraphQL" might make sense but I think our learn docs shouldn't start with REST concepts and map them to GraphQL, but instead start with GraphQL best practices and then if necessary expand with REST accommodations/analogies.

I think my preferred way of addressing this would be to start with a non-CRUD mutation that follows best practices (input object, mutation payload) so that that is front and centre, and then maybe have a sub heading about CRUD that has examples of create, update, upsert, delete. I think the original documentation used a create mutation because that's fairly universal, but perhaps pivoting away from that would help learners to do GraphQL the right way from the start?

Trying to think of something within the Star Wars schema that would make sense as a non-CRUD mutation, perhaps rateFilm which stores a rating "onto the film" from your perspective, but clearly isn't actually updating the film itself:

type Mutation {
  rateFilm(input: RateFilmInput!): RateFilmPayload
}

input RateFilmInput {
  filmId: ID!
  rating: FilmRating! = THUMBS_UP
}

type RateFilmPayload {
  film: Film
}

type Film {
  # ...
  viewerRating: FilmRating
}

enum FilmRating {
  THUMBS_DOWN
  NEUTRAL
  THUMBS_UP
  DOUBLE_THUMBS_UP
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I think this page should talk about the input object/payload best practices at the end, so maybe simplify this down:

type Mutation {
  rateFilm(
    filmId: ID!
    rating: FilmRating! = THUMBS_UP
  ): Film
}

type Film {
  # ...
  viewerRating: FilmRating
}

enum FilmRating {
  THUMBS_DOWN
  NEUTRAL
  THUMBS_UP
  DOUBLE_THUMBS_UP
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can come up with something but at this point I think I'll wait until #1812 is merged so I rebase the changes made in swapi-schema.tsx in that PR first.

Copy link
Member

@benjie benjie Nov 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mandiwise and others added 3 commits November 17, 2024 09:53
Co-authored-by: Benjie <benjie@jemjie.com>
Co-authored-by: Benjie <benjie@jemjie.com>
Co-authored-by: Benjie <benjie@jemjie.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants