Skip to content

docs: editorial for recent documentation updates #4395

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

Open
wants to merge 6 commits into
base: 16.x.x
Choose a base branch
from
Open

Conversation

benjie
Copy link
Member

@benjie benjie commented May 14, 2025

Relevant to these PRs:

In general this content was excellent - thanks @sarahxsanders! - but I wanted to add a few minor updates. I've included comments in a few places with reasoning.

If this PR is too much to review all at once, I'm happy to break it up as I did review of each PR separately.

@benjie benjie requested a review from a team as a code owner May 14, 2025 20:26
@benjie benjie changed the title Light editorial for recent documentation updates docs: editorial for recent documentation updates May 14, 2025

const schema = buildSchema(`
Copy link
Member Author

Choose a reason for hiding this comment

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

The buildSchema approach didn't allow the custom DateTime scalar to be implemented, so I replaced it with the class-based approach.

Comment on lines +195 to 211
const StrictEmailAddress = new GraphQLScalarType({
...EmailAddressResolver,
name: 'StrictEmailAddress',
parseValue(value) {
if (!value.endsWith('@example.com')) {
const email = EmailAddressResolver.parseValue(value);
if (!email.endsWith('@example.com')) {
throw new TypeError('Only example.com emails are allowed.');
}
return email;
},
parseLiteral(literal, variables) {
const email = EmailAddressResolver.parseLiteral(literal, variables);
if (!email.endsWith('@example.com')) {
throw new TypeError('Only example.com emails are allowed.');
}
return EmailAddressResolver.parseValue(value);
return email;
},
Copy link
Member Author

Choose a reason for hiding this comment

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

We commented that it was essential to implement parseLiteral alongside parseValue so I figured we should do that! I also restructured so the underlying scalar does the hard work (especially for literal!) and then we just check the end result. Further, I showed renaming the scalar.

@@ -18,15 +20,15 @@ that works well with clients.

Cursor-based pagination typically uses a structured format that separates
pagination metadata from the actual data. The most widely adopted pattern follows the
[Relay Cursor Connections Specification](https://relay.dev/graphql/connections.htm). While
[GraphQL Cursor Connections Specification](https://relay.dev/graphql/connections.htm). While
Copy link
Member Author

Choose a reason for hiding this comment

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

We renamed this spec to the GraphQL Cursor Connections Specification by agreement at the GraphQL WG a couple years back. Relay hosts it, but it's not really a "relay" spec any more - it's very widely used.

@@ -192,7 +194,7 @@ const usersField = {
let start = 0;
if (args.after) {
const index = decodeCursor(args.after);
if (index != null) {
if (Number.isFinite(index)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This is me being a bit pedantic on the security front, but it's possible that a user could pass something that when you parseInt it would become NaN (e.g. cursor:frog), so the isFinite check ensures that the number is reasonable. Really the hardening should take place in decodeCursor but this felt a cleaner edit.

@@ -2,7 +2,7 @@
title: Solving the N+1 Problem with `DataLoader`
---

When building a server with GraphQL.js, it's common to encounter
When building your first server with GraphQL.js, it's common to encounter
Copy link
Member Author

Choose a reason for hiding this comment

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

I wanted to soften the wording here, don't want people saying "See! Even the GraphQL documentation says that it has major performance issues!" without bothering to read on. Wanted to make this clear this is a mistake you might make early on, but once you've built a few GraphQL schemas using batching will be your bread and butter.

Comment on lines +43 to +44
`source` contains the value returned by the parent object (after resolving any
lists). For root fields, it is the `rootValue` passed to GraphQL, which is often
Copy link
Member Author

Choose a reason for hiding this comment

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

This parenthesis might not be the best way of saying this, but source is essentially the list item inside a list if there is a list, and that's quite important for users to realise.

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.

1 participant