Skip to content

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

Open
wants to merge 55 commits into
base: main
Choose a base branch
from
Open

feat: add eol page #7990

wants to merge 55 commits into from

Conversation

bmuenzenmeyer
Copy link
Collaborator

@bmuenzenmeyer bmuenzenmeyer commented Jul 16, 2025

Description

We now have a dedicated EOL page!

Changes not directly related to the scope

  • We also now have a more generic modal solution
  • Article layouts have site footers.

Validation

EOL Page - live preview at https://nodejs-org-git-eol-openjs.vercel.app/en/eol

image

EOL / Vulnerability Table

image

Details

image

Link updated here

image

and here

image

Vulnerability Blog Posts

image

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.
  • should /eol be in the nav? I did not include it anywhere currently
  • I have read the Contributing Guidelines and made commit messages that follow the guideline.
  • I have run pnpm format to ensure the code follows the style guide.
  • I have run pnpm test to check if all tests are passing.
  • I have run pnpm build to check if the website builds without errors.
  • I've covered new added functionality with unit tests if necessary.

Copy link

vercel bot commented Jul 16, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
nodejs-org ✅ Ready (Inspect) Visit Preview Aug 10, 2025 0:07am

@bmuenzenmeyer
Copy link
Collaborator Author

ran with HUSKY=0 - eslint failures on git commit on Windows. investigating

Copy link

codecov bot commented Jul 16, 2025

Codecov Report

❌ Patch coverage is 93.85965% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.10%. Comparing base (0a9ead8) to head (f579c6f).
⚠️ Report is 13 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
apps/site/next-data/generators/vulnerabilities.mjs 92.68% 6 Missing ⚠️
...ages/ui-components/src/Common/BaseButton/index.tsx 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7990      +/-   ##
==========================================
+ Coverage   73.02%   73.10%   +0.07%     
==========================================
  Files          95       97       +2     
  Lines        8324     8441     +117     
  Branches      214      228      +14     
==========================================
+ Hits         6079     6171      +92     
- Misses       2244     2269      +25     
  Partials        1        1              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ovflowd
Copy link
Member

ovflowd commented Jul 17, 2025

I feel this page should use a layout without the sidebar/metabar FYI

@bmuenzenmeyer
Copy link
Collaborator Author

bmuenzenmeyer commented Jul 17, 2025

I feel this page should use a layout without the sidebar/metabar FYI

that's fine - i just grabbed one quick - using page the default, does not render correctly (should open an issue for that. so using article for now)

this is page/default (not used anywhere)

image

@ovflowd
Copy link
Member

ovflowd commented Jul 19, 2025

@avivkeller can we use the new website for CVEs? https://www.cve.org/CVERecord?id=CVE-2025-23166 instead of cve.mitre.org?

@ovflowd
Copy link
Member

ovflowd commented Jul 19, 2025

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.

@ovflowd

This comment was marked as resolved.

@ovflowd

This comment was marked as resolved.

@ovflowd

This comment was marked as resolved.

@ovflowd

This comment was marked as resolved.

@bmuenzenmeyer

This comment was marked as resolved.

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

I'm still unsatisfied with the state of how we're doing Modals, I attempted now to provide some good examples with reasoning here. Please let's simplify things and do a few rounds of self-review 🙇

We're almost there, I can feel it <3

<ReleaseModal
release={release}
open={currentModal === release.version}
onOpenChange={open => open || setCurrentModal(undefined)}
Copy link
Member

Choose a reason for hiding this comment

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

Maybe the modalProps could just be onClose since we know when it is open or not ;)

So tha onClose becomes just a fn

type ReleaseModalProps = {
isOpen: boolean;
closeModal: () => void;
type ReleaseModalProps = ComponentProps<typeof Modal> & {
Copy link
Member

Choose a reason for hiding this comment

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

We still can have a shared prop for ModalProps fyi that accepts a generic ✨

<AlertBox level="warning">
{t('components.eolAlert.intro')}{' '}
<Link href="/eol">
OpenJS Ecosystem Sustainability Program{' '}
Copy link
Member

Choose a reason for hiding this comment

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

This whole thing should be an i18n string. Even more due to RTL languages.

const t = useTranslations();
return (
<AlertBox level="warning">
{t('components.eolAlert.intro')}{' '}
Copy link
Member

Choose a reason for hiding this comment

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

Actually this whole thing could be an i18n string. And you pass the Link as a reference. Like we do with our rich text i18n strings ;)

};

const EOLModal: FC<EOLModalProps> = ({
release,
Copy link
Member

Choose a reason for hiding this comment

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

Could we destruct this to:

Suggested change
release,
{ major, codename },

const t = useTranslations();

const modalHeading = release.codename
? t('components.eolModal.title', {
Copy link
Member

@ovflowd ovflowd Aug 10, 2025

Choose a reason for hiding this comment

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

Which would allow us to do:

t('components.eolModal.title', { major, codename })

SEVERITY_ORDER.indexOf(a.severity) -
SEVERITY_ORDER.indexOf(b.severity)
),
[vulnerabilities]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
[vulnerabilities]
[vulnerabilities.length]

Should be enough and less expensive

@@ -0,0 +1,78 @@
'use client';
Copy link
Member

@ovflowd ovflowd Aug 10, 2025

Choose a reason for hiding this comment

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

This shouldn't be a client component, does it really need to be? It forces the provdeVulnerabilities and provideReleaseData to run on client-side too.

+ Even if EOLModal is a client component, that's completely fine, as it will just slot in, afaik.

Copy link
Member

Choose a reason for hiding this comment

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

I see, that's due to useState.... 🤔

In this case, I wonder how this will behave. It will probably run these fetch() requests on the client-side, which we want to avoid.

Copy link
Member

Choose a reason for hiding this comment

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

If we move'em to another sub-component, we'll have the same situation as of: React splitting the prop data into the client-side.

At the very least Next.js will do an initial hydration, so there won't be load times for users to see this. But these requests will definitely run every time they open this page....

Copy link
Member

Choose a reason for hiding this comment

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

image

Yeah it is requesting on every load, although it does then cache and load from disk cache. My wonder is:

  • Knowing all is behind CDN, it shouldn't be an issue, and it fetches just in case the state is different, which would trigger a re-render
    • This could though create a situation of React complaining server content !== client content
    • But this should be fine due to skew protection and worst case, just re-renders.

Copy link
Member

Choose a reason for hiding this comment

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

The only thought here is, do we have other pages that are also running these fetch requests on client-side? During the development of this website, we made a conscious effort for all these requests to only be server-side, should we be fine with them being client-side? Instead of having mixed JSONs within the bundle?

import provideVulnerabilities from '#site/next-data/providers/vulnerabilities';
import { EOL_VERSION_IDENTIFIER } from '#site/next.constants.mjs';

import EOLModal from '../EOLModal';
Copy link
Member

Choose a reason for hiding this comment

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

Full import descriptors please 🙇

@ovflowd
Copy link
Member

ovflowd commented Aug 10, 2025

image

FYI getting this on preview, we might want to change the Link to HeroDevs to a regular <a> as it is an external link

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.

Dedicated EOL page Move the homepage security link to the banner