-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Add and update the docs for "Defining a Schema" and "Fetching Data" #6776
Conversation
|
✅ Deploy Preview for apollo-server-docs ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 199f88e:
|
Hm, currently it's challenging to pick out the parts of this PR that differ from the AS3 docs, versus what's just being re-added verbatim. Other reviewers might have a thought here, but what if, before looking at this PR, we merged a different PR that just added these articles to the AS4 docs exactly as they're written in AS3? That would cause the diff for this PR to be limited to the portions that actually changed for v4 |
You make a great point @StephenBarlow!
The only thing that would worry me about the above approach is that folks could navigate to the outdated AS3 docs on the V4 branch of the docsite if they enter the direct URL. But, I feel like that's fine 🤷♀️. @trevor-scheer and @glasser thoughts? I can handle merging in those v3 docs if we feel this is the way to go! |
9d1955b
to
9ac9a87
Compare
Okay, @StephenBarlow and @glasser, this is ready for review now—I merged in the AS3 docs, so we now have the diffs between the two to go off! :) |
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.
Here's the first few files.
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.
woo, schema part is (unsurprisingly) less changey
|
||
7. Finally, ensure you are `listen`ing to your `httpServer`. | ||
|
||
Most Express applications call `app.listen(...)`, but for your setup change this to `httpServer.listen(...)` using the same arguments. This way, the server starts listening on the HTTP and WebSocket transports simultaneously. | ||
Most Express applications call `app.listen(...)`, but for your setup change this to `httpServer.listen(...)` using the same arguments. This way, the server starts listening on the HTTP and WebSocket transports simultaneously. | ||
|
||
A completed example of setting up subscriptions is shown below: |
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.
Do you think it's actually useful that we walk through the code bit by bit and then show it all again here? Maybe just showing it once (fully complete) and not walking through (with extra comments added) would be better?
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, it does feel like a bit much, but on the other hand, I could see this being confusing to implement, and at least this way, we know folks can't miss a step. I could go either way on this, honestly.
@@ -19,8 +19,11 @@ type Query { | |||
|
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.
This is yet another file documenting somebody else's software. (In this case, software that used to be part of AS, then got factored out into a separate library, then the library got passed off to other people, and finally we removed the default integration.) I personally wish we didn't have so much of that, or that it was somewhere separate from the core AS docs. Maybe we consider leaving this file out for v4 and if there's popular demand we resurrect it?
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.
For this article, what if we left it out of the sidebar for now? So we'd still have the file on hand in case people want it, and folks searching for "creating directives" would still find something for their search, but the article is altogether less visible?
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.
That seems a bit confusing to me. Is this entirely about wanting to optimize SEO? Frankly I think people looking for documentation on this functionality of @graphql-tools/schema
would be best served by finding the docs for @graphql-tools/schema
.
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.
It's more like I already put in the effort to update this article and the corresponding example, so the lift has already been done on our side of things. So if that can help people searching in our documentation, that'd be nice? I can remove it if you feel like it doesn't add value though!
I could alternatively remove this full article, but add a plug for the docs-example
in the main Directives article for those interested?
I don't have strong feelings here, so I'm down to do whatever you think is best!
also, at a high level, most of the stuff in the schema/resolvers pages should ideally be TS-first with codegen! but fine if that's a separate project that comes after "restore and correct docs". |
docs/source/data/errors.mdx
Outdated
}); | ||
``` | ||
|
||
To mask or transform errors, you can pass a function to the `sendErrorsInTrace.transform` option for the [usage reporting plugin](/apollo-server/api/plugin/usage-reporting/): |
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.
This doesn't seem to match the code snippet after it?
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.
I clarified this by splitting it out:
To send all errors to Studio you can pass
{ unmodified: true }
tosendErrorsInTraces
, like so:
new ApolloServer({
// etc.
plugins: [
ApolloServerPluginUsageReporting({
// If you pass unmodified: true to the usage reporting
// plugin, Apollo Studio receives ALL error details
sendErrorsInTraces { unmodified: true },
}),
],
});
If you want to report specific errors or modify an error before reporting it, you can pass a function to the sendErrorsInTraces.transform
option, like so:
new ApolloServer({
// etc.
plugins: [
ApolloServerPluginUsageReporting({
sendErrorsInTraces {
transform: (err) => {
if (err.extensions.code === 'MY_CUSTOM_CODE') {
// returning null will skip reporting this error
return null;
}
// All other errors are reported.
return err;
},
},
}),
],
});
This PR updates the articles for the "Defining a Schema" and "Fetching Data" docs sections (minus the Data sources article, which I'll update once data sources are working)
Accompanying updated docs-examples
Related to: #6758