redwood icon indicating copy to clipboard operation
redwood copied to clipboard

Fix redwood root schema definition link on GraphQL docs page

Open Philzen opened this issue 2 years ago • 10 comments

Just stumbled over this 404 on https://redwoodjs.com/docs/graphql#the-root-schema

Philzen avatar Jul 10 '22 00:07 Philzen

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...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

netlify[bot] avatar Jul 10 '22 00:07 netlify[bot]

@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.

Philzen avatar Jul 10 '22 00:07 Philzen

@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:

grafik

Which makes sense. When linking to a file i can see these two options:

  1. 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
  2. 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

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 avatar Jul 12 '22 18:07 Philzen

@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!

dthyresson avatar Jul 25 '22 18:07 dthyresson

☁️ 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.

nx-cloud[bot] avatar Jul 25 '22 20:07 nx-cloud[bot]

@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?

Philzen avatar Jul 27 '22 15:07 Philzen

@jtoar @dthyresson any remaining concerns with the amendments i pushed as per your requests 11 days ago?

Philzen avatar Aug 08 '22 01:08 Philzen

@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.

Tobbe avatar Aug 08 '22 01:08 Tobbe

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

Philzen avatar Aug 08 '22 23:08 Philzen

OK to merge?

Philzen avatar Aug 09 '22 17:08 Philzen