feat: Introduce Downloads Archive page #7794
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new simplified downloads page that’s fully server-rendered (no JS required) and provides major‐version download summaries, with expandable minor versions under <details>. It also centralizes download utilities and updates the download URL API to use an options object.
- Introduce
simplified.mdxandDownloadSimpleLayoutfor the new page - Update
getNodeDownloadUrlsignature and its callers to use an options object - Add
DownloadsTable,Details, andWithSimplifiedDownloadcomponents
Reviewed Changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| util/downloadUtils/index.tsx | Exported DownloadDropdownItem type |
| util/downloadUtils/constants.json | Added compatibility ranges for platforms |
| util/tests/getNodeDownloadUrl.test.mjs | Updated tests for new getNodeDownloadUrl signature |
| types/layouts.ts | Added 'download-simple' layout type |
| pages/en/download/simplified.mdx | New MDX page with simplified download UI |
| next.mdx.use.mjs | Registered new MDX components (WithSimplifiedDownload, etc.) |
| next.dynamic.constants.mjs | Configured dynamic routing and exclusions for new page |
| layouts/DownloadSimple.tsx | New layout wrapping simplified download content |
| components/withSimplifiedDownload.tsx | HOC to provide download data and sidebar items |
| components/withProgressionSidebar.tsx | Refactored to accept both navKey and explicit groups |
| components/withMetaBar.tsx | Made items injectable for meta‐bar |
| components/withMarkdownContent.tsx | New HOC to load MDX content at runtime |
| components/withLayout.tsx | Added DownloadSimpleLayout to layout mapping |
| components/MDX/Details | New <Details> component for collapsible sections |
| components/Downloads/Release/ReleaseCodeBox.tsx | Updated “no-script” link to point at new simplified page |
| components/Downloads/Release/PrebuiltDownloadButtons.tsx | Switched getNodeDownloadUrl calls to object syntax |
| components/Downloads/DownloadsTable | New table for listing artifacts |
| components/Downloads/DownloadLink.tsx | Updated to new getNodeDownloadUrl API |
| components/Downloads/DownloadButton | Updated to new getNodeDownloadUrl API |
Comments suppressed due to low confidence (1)
apps/site/components/Downloads/DownloadsTable/index.tsx:1
- Consider adding unit tests for
DownloadsTableto verify it renders rows correctly for differentsourceinputs.
'use client';
Co-authored-by: Copilot <[email protected]> Signed-off-by: Caner Akdas <[email protected]>
|
@canerakdas What's the status here? |
I’m waiting for more feedback and reviews / approval 😄 |
| @@ -22,11 +22,21 @@ const PrebuiltDownloadButtons: FC = () => { | |||
| const { release, os, platform } = useContext(ReleaseContext); | |||
|
|
|||
| const installerUrl = platform | |||
| ? getNodeDownloadUrl(release.versionWithPrefix, os, platform, 'installer') | |||
| ? getNodeDownloadUrl({ | |||
There was a problem hiding this comment.
OOC, why is this function now taking an object?
There was a problem hiding this comment.
Some parameters in this function are optional for example, when generating a shasum or source link, providing only the version and kind is sufficient. Instead of calling
getNodeDownloadUrl(version, undefined, undefined, 'source')it feels more readable to use a named parameter approach;
getNodeDownloadUrl({ version, kind: 'source' })
|
|
||
| import { dynamicRouter } from '#site/next.dynamic'; | ||
|
|
||
| const getMarkdownContent = async (locale: string, file: Array<string>) => { |
|
|
||
| <div className={styles.downloadLayout}> | ||
| <main> | ||
| <WithMarkdownContent file={['download', 'archive']} /> |
There was a problem hiding this comment.
Why is this over here and not coming from the dynamic router?
| @@ -29,6 +32,14 @@ export const IGNORED_ROUTES = [ | |||
| * @type {Map<string, import('./types').Layouts>} A Map of pathname and Layout Name | |||
| */ | |||
| export const DYNAMIC_ROUTES = new Map([ | |||
| // Creates dynamic routes for downloads archive pages for each version | |||
| // (e.g., /download/v18.20.8, /download/v20.19.2) | |||
| ...provideReleaseData() | |||
There was a problem hiding this comment.
My fear is that this will blow static builds in a monstruous way.
There was a problem hiding this comment.
Also calling the provider right here in a constant file that could be called in a non-react environment might not be a good idea.
There was a problem hiding this comment.
Do you have a number of how many more routes this generates?
There was a problem hiding this comment.
Currently, there are 606 pages for the default locale alone. Should all of them be included in the static build, or should we selectively build only the ones that are most frequently accessed (lts, current, or last X version)?
There was a problem hiding this comment.
You can do the following on the ignore list:
- locale isn't defaultLocale and paths starts with /download/archive
There was a problem hiding this comment.
I wonder how long the static build takes in this branch?
There was a problem hiding this comment.
Speed seems actually quite ok. Btw seeing this error on the build "Error: Cannot apply utility class after:md:-left-1/2 because the md variant does not exist."
There was a problem hiding this comment.
Speed seems actually quite ok. Btw seeing this error on the build "Error: Cannot apply utility class
after:md:-left-1/2because themdvariant does not exist."
Rebase, it's been fixed
| file: Array<string>; | ||
| }; | ||
|
|
||
| const WithMarkdownContent: FC<WithMarkdownContentProps> = async ({ file }) => { |
There was a problem hiding this comment.
I can only imagine that your markdown content is not being rendered, but probably because the next.dynamic.router was designed for the generated routes to not include markdown, maybe that can be tweaked. I don't think we should have this WithMarkdownContent component, and have yet another place that can read files and markdown. That could further complicate things for Open Next (Cloudflare)
| status: NodeReleaseStatus; | ||
| }; | ||
|
|
||
| const WithReleaseAlertBox: FC<WithReleaseAlertBoxProps> = ({ status }) => { |
| return ( | ||
| <AlertBox | ||
| title={t('components.common.alertBox.info')} | ||
| level="info" |
There was a problem hiding this comment.
Can we creeate a green alert box variant and use this one here?
| @@ -70,8 +70,12 @@ const generateReleaseData = async () => { | |||
| const status = getNodeReleaseStatus(new Date(), support); | |||
|
|
|||
| const minorVersions = Object.entries(major.releases).map(([, release]) => ({ | |||
There was a problem hiding this comment.
You know the more data you add, the bigger the whole thing becomes. I'm only afraid on bundle size changes and how bigger of a footprint this change especifically will do on our client-bundles and HTMLs
| @@ -19,6 +20,8 @@ export const IGNORED_ROUTES = [ | |||
| locale !== defaultLocale.code && /^blog/.test(pathname), | |||
| // This is used to ignore all pathnames that are empty | |||
| ({ locale, pathname }) => locale.length && !pathname.length, | |||
| // This is used to ignore download routes for Node.js versions and downloads archive page | |||
| ({ pathname }) => /^download\/(v\d+(\.\d+)*|archive)$/.test(pathname), | |||
There was a problem hiding this comment.
Do we need this ignore? btw instead of the URLs being /download/vXX/archive it should be /download/archive/{xVV} this would probably remove the need to also have this ignore.
There was a problem hiding this comment.
Is this /download/v{XX} and /download/archive?
There was a problem hiding this comment.
I feel the /download/archive url should probably not exist. As it could be ambiguous with /current and /lts
There was a problem hiding this comment.
It'd only make sense if we had a /download/archive and /download/archive/v{XX}
There was a problem hiding this comment.
Is this
/download/v{XX}and/download/archive?
This is intended for /download/v{XX} and /download/archive, where /download/archive represents the LTS release. I think it could be useful in places where we want to provide a fixed link
It'd only make sense if we had a
/download/archiveand/download/archive/v{XX}
I also think the page URLs should follow this format. I’ll look into removing them from the ignore list (I might have misunderstood part of the issue description)
There was a problem hiding this comment.
It doesn't make sense for /download/archive be the LTS page when /download is the LTS page.
IMO the /download/archive/{version} makes more sense for me, and /download/archive could redirect to /download/archive/{latestVersion} (not LTS)
The reason we want /download go to LTS is just that for newcomers that is the version we want to direct people towards. But the archive entry point should always be the latest version. And the redirect should be so that users get redirected for what was at the point they accessed the latest version
| // Creates dynamic routes for downloads archive pages for each version | ||
| // (e.g., /download/v18.20.8, /download/v20.19.2) | ||
| ...provideReleaseData() | ||
| .flatMap(({ minorVersions, versionWithPrefix }) => [ |
There was a problem hiding this comment.
Instead of a flatMap make two separate maps for readability, as we did for the blog categoreis and posts... Also instead of relying on provideReleaseData() maybe the providers could have a simple provideReleaseVersions() including all our versions.
github-actions
bot
removed
the
github_actions:pull-request
|
Lighthouse Results
|
|
|
||
| <h2 className="flex items-center gap-2"> | ||
| <img | ||
| src="/static/images/favicons/favicon.png" |
Description
This PR introduces a new statically generated page that displays simplified download options by major version:
Also includes a download table of other minor versions of the major version;
Since I included the minor versions as a table, the content became quite lengthy. To prevent this, I used the HTML
detailselement. The main reason I didn't use Radix Primitive Accordion is that we want this page to be fully usable even for users who have JavaScript disabledI am open to suggestions for to add / remove content 🙇
Validation
The links below are accessible in the preview;
Related Issues
Addresses #7443
Check List
pnpm formatto ensure the code follows the style guide.pnpm testto check if all tests are passing.pnpm buildto check if the website builds without errors.