-
-
Notifications
You must be signed in to change notification settings - Fork 6.4k
feat: add eol page #7990
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
base: main
Are you sure you want to change the base?
feat: add eol page #7990
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
ran with |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7990 +/- ##
==========================================
- Coverage 73.02% 72.91% -0.12%
==========================================
Files 95 96 +1
Lines 8324 8359 +35
Branches 214 214
==========================================
+ Hits 6079 6095 +16
- Misses 2244 2263 +19
Partials 1 1 ☔ View full report in Codecov by Sentry. |
I feel this page should use a layout without the sidebar/metabar FYI |
@avivkeller can we use the new website for CVEs? https://www.cve.org/CVERecord?id=CVE-2025-23166 instead of cve.mitre.org? |
Also this description: "There are 4+ known vulnerabilities associated with this Node.js release. Please review their severity and details to understand the potential impact." not sure if it is fitting. I think we should lean more into what CVEs are how to understand them what having these issues are... IDK; Throwing the "Please review their severity and details to understand the potential impact." to the end-user might not be ideal. I mean in the end that's whaty they're going to do anyways, but we could use such section to explain actual concrete details of what CVEs means, what clicking these links means, idk. |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
@@ -150,7 +150,9 @@ const getPage: FC<DynamicParams> = async props => { | |||
// within a server-side context | |||
return ( | |||
<MatterProvider {...sharedContext}> | |||
<WithLayout layout={frontmatter.layout}>{content}</WithLayout> | |||
<WithLayout layout={frontmatter.layout} modal={frontmatter.modal}> |
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 don't like this approach of modal metadata to be injected at layout-level, it implies you cannnot have different sort of modals per page.
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.
Even less making it part of the frontmatter, which is even worse IMO.
const LayoutComponent = layouts[layout] ?? DefaultLayout; | ||
|
||
if (modal) { |
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 don't think it should be responsibility of the Layout to surround itself with a provider.
You should surround the Modal itself with its provider. And the ModalProvider should probably not exist or not be needed to provide data. See for our example our WithRelease
provider that allows you to inject the context itself to children.
const majorVersions = | ||
vulnerability.vulnerable | ||
.match(/\b\d+\b/g) | ||
?.map(Number) |
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 an odd way of extracting this, I think you can extract the matcher to the assignment of majorVersions to make it cleaner
?.map(Number) | ||
.filter(major => !isNaN(major)) ?? []; | ||
|
||
for (const majorVersion of majorVersions) { |
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 feels that these 2-dimensional for could be a array.reduce tbh
@@ -1,10 +1,13 @@ | |||
--- | |||
title: Node.js Releases | |||
layout: about | |||
modal: release |
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.
-1 for this approach. It hinders the ability of having multiple kind of modals per page and feels like an odd architectural choice.
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.
Still strong -1 on this approach. This ain't a provider, this feels like a factory that you're injecting on layout-level which is egredgius at best.
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.
Design-wise? ✔️, Content-Wise? ✔️, Code-wise/Architecture ❌
Description
We now have a dedicated EOL page!
Changes not directly related to the scope
Validation
EOL Page - live preview at https://nodejs-org-git-eol-openjs.vercel.app/en/eol
EOL / Vulnerability Table
Details
Link updated here
and here
Vulnerability Blog Posts
Related Issues
closes #7906
closes #7899
Check List
All other links on all alert boxes across the website (blog post from Matteo, Download pages, Version modal, etc) go to the /EOL page
- did not do Matteo's blog post yet - would rather someone else choose to editorialize that.pnpm format
to ensure the code follows the style guide.pnpm test
to check if all tests are passing.pnpm build
to check if the website builds without errors.