redwood
redwood copied to clipboard
Fix redwood root schema definition link on GraphQL docs page
Just stumbled over this 404 on https://redwoodjs.com/docs/graphql#the-root-schema
Deploy Preview for redwoodjs-docs ready!
Name | Link |
---|---|
Latest commit | 3481fc2d829dbfd3cf73da92d6bbaa8de6de889a |
Latest deploy log | https://app.netlify.com/sites/redwoodjs-docs/deploys/62f7dab9fc19a700089b6613 |
Deploy Preview | https://deploy-preview-5896--redwoodjs-docs.netlify.app |
Preview on mobile | Toggle QR Code...Use your smartphone camera to open QR code link. |
To edit notification comments on pull requests, go to your Netlify site settings.
@dthyresson i realized the file packages/api/src/makeMergedSchema/rootSchema.ts
does not longer exist in main
and must have been moved somewhere completely else. Is it still correct (schema-wise) to refer to the 34a6444432b409774d54be17789a7109add9709a blob or should that link be updated to a different branch ?
Kindly advise.
@jtoar Regarding your review comment posting this here b/c this i feel this is profound stuff (that i never really thought about before, until this PR):
I made a permalink to the current file in main (also discovered that functionality just today) like this:
Which makes sense. When linking to a file i can see these two options:
- linking to the file (in this case including line no. highlighting) in a branch, e.g.
main
.- Pro:
- (theoretically) the link always leads to the most recent version, given the file is not moved
- Con:
- When that file is changed, the line numbers are not anymore referring to what was intended to show here
- When that file is moved the link goes dead / 404
- Pro:
- Using a permalink
- Pro:
- Highlighted line numbers always accurately map to what was intended to show
- Link can never go dead / 404 if the file is moved
- Con:
- Not always pointing to the most recent version
- Pro:
Which scenario would you prefer?
If you look at the current doc, one link is dead (which is the reason for this PR), the other one still works, which is a permalink.
Hence, it transpires to me that it's always preferable to use permalinks when writing docs that point to specific sections in the code – b/c that way the the documentation is in sync with the code as it was at the time of writing. If time moves on and there were major changes in the linked code, the documentation is likely to also need update.
@Philzen The Core Team discussed this PR and it gave us a chance to re-think how we permalink to code in the docs.
Because these links are for a specific commit, if we change the root schema (which it will do in an upcoming merge to add comments), then the links are out of date.
Instead we'd rather:
- Link to the main file https://github.com/redwoodjs/redwood/blob/main/packages/graphql-server/src/rootSchema.ts
- Embed fenced code snippets .... which we would change as part of any new PR and documentation needs.
If you would be able to make those changes let me know, or re-assign and I will assume this PR and make the changes.
Thanks!
☁️ Nx Cloud Report
CI is running/has finished running commands for commit 3481fc2d829dbfd3cf73da92d6bbaa8de6de889a. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.
📂 See all runs for this branch
✅ Successfully ran 8 targets
Sent with 💌 from NxCloud.
@Philzen The Core Team discussed this PR and it gave us a chance to re-think how we permalink to code in the docs.
Because these links are for a specific commit, if we change the root schema (which it will do in an upcoming merge to add comments), then the links are out of date.
@dthyresson my concern would be that when the file is changed the links refer to the wrong code lines, or if the file is moved, they will become 404 – bringing us to the same situation that led to this PR.
But as the core team has found this consensus i'm happy to follow along to get this off the table.
Instead we'd rather:
* Link to the main file https://github.com/redwoodjs/redwood/blob/main/packages/graphql-server/src/rootSchema.ts
→ eb7829570bb5a134af56ae84ad8cb04742c44d1d
* Embed fenced code snippets .... which we would change as part of any new PR and documentation needs.
→ 8b0aa716242555d81282aa1c52ae7576bf3818f2
Is this what you intended or should we wrap it into a <details>
-section maybe to keep the doc page a little more brief at first glance?
@jtoar @dthyresson any remaining concerns with the amendments i pushed as per your requests 11 days ago?
@Philzen you wrote
@dthyresson my concern would be that when the file is changed the links refer to the wrong code lines, or if the file is moved, they will become 404 – bringing us to the same situation that led to this PR.
Now I don't remember exactly what we said during that core team discussion. But it could have been that we said to just link to the file, not any specific line numbers. Then include the fenced code blocks to point out exactly what lines we mean. That would remove half of the problem you've stated. The other half of the problem is more fun, in that I think it would be catchable in CI right? If we had a job to go through the docs and check all links for 404.
The other half of the problem is more fun, in that I think it would be catchable in CI right? If we had a job to go through the docs and check all links for 404.
I was actually going to suggest exactly that in a separate RFC, i already have it in my notes :wink:
But it could have been that we said to just link to the file, not any specific line numbers.
Fair enough, now updated accordingly via fb622b2b9
OK to merge?