Skip to content

feat(doc-kit): bump version, use virtualImports#29

Open
avivkeller wants to merge 4 commits intomainfrom
doc-kit-improvements
Open

feat(doc-kit): bump version, use virtualImports#29
avivkeller wants to merge 4 commits intomainfrom
doc-kit-improvements

Conversation

@avivkeller
Copy link
Copy Markdown
Member

Updates doc-kit to the latest version, adding support for:

  1. Custom canonical links (<link rel="canonical" href="https://nodejs.org/learn{path}" />) (cc @MattIPv4)
  2. Loading navigation and sidebar from file via virtualImports (cc @ovflowd)

@vercel
Copy link
Copy Markdown

vercel bot commented Apr 4, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
nodejs-learn Ready Ready Preview Apr 4, 2026 6:41pm

Request Review

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Updates the Learn site’s doc-kit setup to the latest version and shifts navigation/canonical behavior to be driven by a shared template and virtual JSON imports, aligning Learn with nodejs.org site navigation data.

Changes:

  • Bump @node-core/doc-kit to 1.3.2 and configure templatePath + canonical link rendering.
  • Introduce build-time virtualImports for #site/navigation.json (from nodejs.org) and #learn/navigation.json (derived from local navigation.json + page titles).
  • Refactor Sidebar/Nav/Footer components to consume the virtual navigation JSON instead of hardcoded/static local data.

Reviewed changes

Copilot reviewed 10 out of 11 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
utils/index.mjs Adds helpers to derive sidebar groupings + i18n population used by virtual imports.
template.html New HTML template including canonical link and updated head tags.
package.json Bumps doc-kit devDependency to 1.3.2.
package-lock.json Lockfile update for doc-kit 1.3.2.
navigation.json Adds local Learn sidebar grouping source (paths per category).
doc-kit.config.mjs Configures template and adds virtualImports for site + learn navigation.
components/Sidebar/index.jsx Switches sidebar groups to #learn/navigation.json virtual import.
components/Navigation/index.jsx Switches top nav items to #site/navigation.json virtual import.
components/Footer/index.jsx Switches footer links/social links to #site/navigation.json virtual import.
components/Footer/footer.json Removes now-redundant local footer navigation JSON.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Copy Markdown
Member

@ovflowd ovflowd left a comment

Choose a reason for hiding this comment

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

Left a few comments.

'#site/navigation.json': JSON.stringify(
populateI18n(
await fetch(
'https://raw.githubusercontent.com/nodejs/nodejs.org/refs/heads/main/apps/site/navigation.json'
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could this pretty please be a constant?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why we adding a navigation.json ref? When we also adding a navigation.json file?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

#site/navigation is for the navbar and footer
#learn/navigation is for the sidebar

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Constant please and inline doc explaining the diff

),
}));

export const createI18nPopulator = async () => {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No, let's not mess with anything i18n here, completely out of scope, please.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We are fetching the site navigation from https://github.com/nodejs/nodejs.org/blob/main/apps/site/navigation.json, which needs i18n resolved.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hard no for this, we shouldn't use the navigation from there NOR use i18n here. These are separate projects, let's not link them. If we ever need to update the Navigation there, we can just open a PR updating it here too.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's please remove this dependency.


export const PAGES_DIR = join(import.meta.dirname, '../pages');

export const getTitle = async path => {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hmmm, why?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Rather than specifying the title both in the page and the config, we can just get it from the page, no?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's please not. Let's follow what we've been doing already, manually specifying the title, this is supposed to be as static and manual as possible, let's not add automatic magic things.

On nodejs/node sidebar titles !== content title, same on Node.js org, so let's also manually define them here 🙇

Copy link
Copy Markdown
Member

@MattIPv4 MattIPv4 left a comment

Choose a reason for hiding this comment

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

👀 https://nodejs-learn-git-doc-kit-improvements-openjs.vercel.app/learn/ has a canonical of <link rel="canonical" href="https://nodejs.org/learn/index">, that isn't right.

@avivkeller
Copy link
Copy Markdown
Member Author

Okay, I need to fix doc-kit to strip the /index, reverting that change. I'm also going to add a useAbsoluteURLs config to doc-kit which will resolve all of our relative URLs hacks.

In the meantime, fixed Vercel config for more relative URL issues

Signed-off-by: Aviv Keller <me@aviv.sh>
@avivkeller
Copy link
Copy Markdown
Member Author

See: nodejs/doc-kit#751

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why are these pages special? Not sure I'm a fan of having complex redirect rules like this?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Nor am I, we can remove them after nodejs/doc-kit#751. These pages are all /index.html, and need the forward slash until we stop using relative URLs in 751.

Copy link
Copy Markdown
Member

@MattIPv4 MattIPv4 Apr 4, 2026

Choose a reason for hiding this comment

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

Why are they /index.html pages though? Why aren't they being generated as <path>.html like all our other pages?

Absolute URL support fixes this, sure, but doesn't actually address why these are being generated differently from all the other pages?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

They aren't generated differently, they are written as index.md pages in the source content, for instance https://github.com/nodejs/learn/blob/main/pages/diagnostics/memory/index.md

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That seems like something doc-kit should be able to handle then, and update its imports based on the fact that it knows it has generated an index instead of a path?

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