Skip to content

feat(lint): use ESLint for linting snippets #7720

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

Merged
merged 1 commit into from
May 12, 2025

Conversation

avivkeller
Copy link
Member

Description

This PR replaces our custom snippet linting logic with MDX's lintCodeBlocks option.

Validation

Linting should pass

Check List

  • 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.

@Copilot Copilot AI review requested due to automatic review settings May 3, 2025 15:15
@avivkeller avivkeller requested review from a team as code owners May 3, 2025 15:15
Copy link

vercel bot commented May 3, 2025

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

Name Status Preview Updated (UTC)
nodejs-org ✅ Ready (Inspect) Visit Preview May 9, 2025 10:25pm

Copy link
Contributor

github-actions bot commented May 3, 2025

Note

Your Pull Request seems to be updating Translations of the Node.js Website.

Whilst we appreciate your intent; Any Translation update should be done through our Crowdin Project.
We recommend giving a read on our Translation Guidelines.

Thank you!

Copy link
Contributor

@Copilot 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

This PR refactors our code snippets by replacing custom linting logic with MDX’s built‐in lintCodeBlocks option, while also updating variable declarations from var to const/let and reordering some imports.

  • Replace custom snippet linting with MDX’s lintCodeBlocks
  • Switch from var to const/let in multiple code examples
  • Reorder and adjust import statements in several release and documentation pages

Reviewed Changes

Copilot reviewed 46 out of 48 changed files in this pull request and generated no comments.

Show a summary per file
File Description
apps/site/pages/en/learn/asynchronous-work/dont-block-the-event-loop.md Convert variable declarations to const and improve logging
apps/site/pages/en/learn/asynchronous-work/discover-promises-in-nodejs.md Use const for promise executor variables
apps/site/pages/en/learn/asynchronous-work/asynchronous-flow-control.md Remove unnecessary ESLint disable comments; use const in loops
apps/site/pages/en/blog/release/*.md Reorder and adjust import statements and update logging output
apps/site/pages/en/blog/module/service-logging-in-json-with-bunyan.md Replace var with const; update logging usage/comments
apps/site/pages/en/blog/feature/streams2.md Replace var with const; modernize event handler callbacks
apps/site/pages/en/blog/community/domain-postmortem.md Replace var with let and update loop declarations; update error handling comments
apps/site/pages/en/blog/announcements/v20-release-announce.md Reorder import statements to reflect recommended ordering
apps/site/next.mdx.plugins.mjs Remove redundant comments in plugin definitions
apps/site/eslint.config.js Add MDX remark processor configuration and mdx.flatCodeBlocks setting
Files not reviewed (2)
  • apps/site/package.json: Language not supported
  • apps/site/pages/en/index.mdx: Language not supported
Comments suppressed due to low confidence (1)

apps/site/pages/en/blog/community/domain-postmortem.md:194

  • Consider using Buffer.alloc(FILESIZE) instead of Buffer(FILESIZE) for enhanced memory safety.
const buf = Buffer(FILESIZE);

@codecov-commenter
Copy link

codecov-commenter commented May 3, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.84%. Comparing base (10a07a1) to head (c591cc5).
Report is 1 commits behind head on main.

✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #7720   +/-   ##
=======================================
  Coverage   74.84%   74.84%           
=======================================
  Files          98       98           
  Lines        7888     7888           
  Branches      200      200           
=======================================
  Hits         5904     5904           
  Misses       1983     1983           
  Partials        1        1           

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

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Member

@araujogui araujogui left a comment

Choose a reason for hiding this comment

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

LGTM

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.

LGTM! Just double check all snippets to verify we didn't lose things.

@avivkeller
Copy link
Member Author

Note that the snippets on the main page are no longer uniformly 14 lines after this PR. If that's a problem, I can add an additional comment to the 13-lined snippet to make them uniform again.

@ovflowd
Copy link
Member

ovflowd commented May 4, 2025

Note that the snippets on the main page are no longer uniformly 14 lines after this PR. If that's a problem, I can add an additional comment to the 13-lined snippet to make them uniform again.

Let's open another issue for that, I feel we can change the CSS so the width/height of all code tabs becomes the width/height of the largest one; I recall CSS grid/flexboxes support something like that. Can I create a follow-up issue for this?

@ovflowd ovflowd added the github_actions:pull-request Trigger Pull Request Checks label May 6, 2025
@github-actions github-actions bot removed the github_actions:pull-request Trigger Pull Request Checks label May 6, 2025
Copy link
Contributor

github-actions bot commented May 6, 2025

Lighthouse Results

URL Performance Accessibility Best Practices SEO Report
/en 🟢 99 🟢 100 🟢 100 🟢 91 🔗
/en/about 🟢 100 🟢 100 🟢 100 🟢 91 🔗
/en/about/previous-releases 🟢 99 🟢 100 🟢 100 🟠 83 🔗
/en/download 🟢 99 🟢 100 🟢 100 🟢 91 🔗
/en/blog 🟢 100 🟢 100 🟢 96 🟢 92 🔗

@avivkeller
Copy link
Member Author

Looks like I got some errors to review, will do!

@avivkeller
Copy link
Member Author

@nodejs/web-infra This doesn't really concern the other groups, so I doubt they will review. Can we merge without them?

@avivkeller avivkeller enabled auto-merge May 8, 2025 17:24
@avivkeller
Copy link
Member Author

I've enabled auto-merge. Please merge when ready, with or without additional reviews.

@avivkeller
Copy link
Member Author

Bump @nodejs/releasers @nodejs/security-wg

@richardlau
Copy link
Member

Bump @nodejs/releasers

I'm concerned that a number of release posts have been updated in this PR, which suggests that this is going to be using different linting rules than in nodejs/node (since we lint the changelogs there and the release posts are generated from those).

@ovflowd ovflowd disabled auto-merge May 9, 2025 12:29
@avivkeller
Copy link
Member Author

Thanks! Ill update the globs to only match learn articles

@avivkeller avivkeller force-pushed the lint-snippets-with-eslint branch from b88636b to c591cc5 Compare May 9, 2025 22:24
@avivkeller avivkeller removed the request for review from a team May 9, 2025 22:24
@avivkeller avivkeller added the github_actions:pull-request Trigger Pull Request Checks label May 9, 2025
@github-actions github-actions bot removed the github_actions:pull-request Trigger Pull Request Checks label May 9, 2025
Copy link
Member

@UlisesGascon UlisesGascon left a comment

Choose a reason for hiding this comment

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

Changes in Security best practices are ok for me 👍

@avivkeller avivkeller added this pull request to the merge queue May 12, 2025
Merged via the queue into main with commit 91544e7 May 12, 2025
18 checks passed
@avivkeller avivkeller deleted the lint-snippets-with-eslint branch May 12, 2025 15:06
@ovflowd
Copy link
Member

ovflowd commented May 14, 2025

Bump @nodejs/releasers

I'm concerned that a number of release posts have been updated in this PR, which suggests that this is going to be using different linting rules than in nodejs/node (since we lint the changelogs there and the release posts are generated from those).

@avivkeller did you address this comment?

@avivkeller
Copy link
Member Author

avivkeller commented May 14, 2025

Yes. I changed the glob to skip blog posts.

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.

9 participants