-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
feat: create release schedule component #8529
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?
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
👋 Codeowner Review RequestThe following codeowners have been identified for the changed files: Team reviewers: @nodejs/nodejs-website @nodejs/web-infra Please review the changes when you have a chance. Thank you! 🙏 |
| "feed": "~5.1.0", | ||
| "github-slugger": "~2.0.0", | ||
| "gray-matter": "~4.0.3", | ||
| "lts": "github:araujogui/lts-schedule#refactor-website", |
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.
We can use the upstream package once this pr is merged
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.
Adding blocked until then
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8529 +/- ##
==========================================
+ Coverage 74.70% 74.98% +0.27%
==========================================
Files 102 103 +1
Lines 8956 9042 +86
Branches 305 312 +7
==========================================
+ Hits 6691 6780 +89
+ Misses 2263 2260 -3
Partials 2 2 ☔ View full report in Codecov by Sentry. |
| const sixMonthsFromNow = new Date(); | ||
| sixMonthsFromNow.setMonth(now.getMonth() + 6); | ||
|
|
||
| const svg = create({ |
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 showing a warning that I'm not sure how to solve:
@node-core/website:dev: Package jsdom can't be external
@node-core/website:dev: The request jsdom matches serverExternalPackages (or the default list).
@node-core/website:dev: The package resolves to a different version when requested from the project directory (27.4.0) compared to the package requested from the importing module (9.4.1).
@node-core/website:dev: Make sure to install the same version of the package in both locations.
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.
Pull request overview
This PR replaces the static SVG image of the Node.js release schedule with a dynamically generated component that fetches schedule data from GitHub and renders it using the lts library.
Key Changes:
- Adds a new
ReleaseScheduleReact component that dynamically generates the schedule visualization - Introduces a data provider to fetch release schedule JSON from the Node.js Release repository
- Replaces the static image reference in the MDX file with the new component
Reviewed changes
Copilot reviewed 7 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Adds lts package from GitHub repository along with its dependencies (d3, jsdom, svg2png, etc.) |
| apps/site/package.json | Declares the lts dependency from GitHub repository |
| apps/site/next.constants.mjs | Adds RELEASE_SCHEDULE_URL constant pointing to schedule.json |
| apps/site/next-data/generators/releaseSchedule.mjs | Creates fetch function for release schedule data |
| apps/site/next-data/providers/releaseSchedule.ts | Provides cached access to release schedule data |
| apps/site/components/Releases/ReleaseSchedule/index.tsx | Implements the ReleaseSchedule component with SVG generation |
| apps/site/mdx/components.mjs | Registers ReleaseSchedule component for MDX usage |
| apps/site/pages/en/about/previous-releases.mdx | Replaces static image with ReleaseSchedule component |
| apps/site/next-env.d.ts | Updates TypeScript reference path (appears unintentional) |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return ( | ||
| <div | ||
| dangerouslySetInnerHTML={{ __html: svg.html() }} | ||
| className="h-auto w-auto" | ||
| /> |
Copilot
AI
Jan 9, 2026
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.
The generated SVG HTML is being directly injected using dangerouslySetInnerHTML without any sanitization. While the content comes from the 'lts' library, this could pose security risks if that library or the data it processes is compromised. Consider:
- Validating/sanitizing the SVG output before rendering
- Using a safer rendering method if possible
- Adding comments explaining why dangerouslySetInnerHTML is necessary here
📦 Build Size ComparisonSummary
Changes➕ Added Assets (2)
➖ Removed Assets (2)
|
|
(I'll fix the comparator to comment on fork PRs) |
| jsdom@9.4.1: | ||
| resolution: {integrity: sha512-2M0vE8qIK0YF7apWYe0koqKsnChHKYtFqjoZpc1Mo6141TfoamZaCu6FvDVzXvMKYoFB+xGDuANgGDoyqrBt3Q==} | ||
|
|
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 causing the warning. This JSDOM version is not the same version as the one we use (a few lines above), is there a package importing this version specifically, or would a dedupe fix it?
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.
|
|
||
| return ( | ||
| <div | ||
| dangerouslySetInnerHTML={{ __html: svg.html() }} |
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.
While I understand the reason for this, is there a way to avoid this potentially dangerous situation?
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.
Not that I'm aware of, svg.html() returns a simple string
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 would probably then recommend finding a way to make this return a react node instead of raw html. Who maintains 'lts'? Can we make a @node-core/lts-react?
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's on Node.js domain
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.
We can return variables like xScale, yScale, xAxis etc from the lts function and render the SVG inside our component using jsx.
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.
Who maintains 'lts'
build wg but not really maintained. IMO it's can be a good idea to make it live here under web-infra team
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.
We probably should have co-ownership, but this feels like smth that should be owned by releasers or build primarily.
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.
Who maintains 'lts'
build wg but not really maintained. IMO it's can be a good idea to make it live here under web-infra team
@avivkeller @ovflowd I think @AugustinMauroy has a great point. The @nodejs/releasers showed they already have plans of getting rid of SVG, so maybe it's time for the @nodejs/nodejs-website to create our own version.
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.
@avivkeller I don't think they have plans to get rid of the svg ;) what Richard stated was that the PR Guilherme originally opened didn't make sense for them on their PoV and should be done on our side.
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 we should have competing repositories for the lts svg source of truth tbh. All I'm saying is we can have shared ownership of the lts package if needed. Or make a new package that extends that one at maximum, but I'd like to avoid that route...
| "feed": "~5.1.0", | ||
| "github-slugger": "~2.0.0", | ||
| "gray-matter": "~4.0.3", | ||
| "lts": "github:araujogui/lts-schedule#refactor-website", |
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.
Adding blocked until then
|
|
||
| return ( | ||
| <div | ||
| dangerouslySetInnerHTML={{ __html: svg.html() }} |
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.
to be honest, I'm fine with this as long as server-side-rendering works.
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.
Me too. We can refactor this by creating a component that does not use d3-node, but we'll end up with more duplicated code.
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'm fine too
Description
Create new release schedule component
Validation
Related Issues
Fixes #8101
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.