Skip to content

feat(mdx): move mdx into sub-package #7752

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

Closed
wants to merge 4 commits into from
Closed

feat(mdx): move mdx into sub-package #7752

wants to merge 4 commits into from

Conversation

avivkeller
Copy link
Member

@avivkeller avivkeller commented May 16, 2025

Description

The apps/site directory is currently quite complex. Since we're using a monorepo, why not take advantage of that structure whenever we can? The MDX functionality can be extracted into its own package, which might also be useful for api-docs-tooling down the line.

Validation

Everything should build and run without regressions

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 16, 2025 00:03
@avivkeller avivkeller requested review from a team as code owners May 16, 2025 00:03
Copy link

vercel bot commented May 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 May 18, 2025 11:01pm

Copy link
Contributor

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 the MDX functionality by extracting it into its own package and making several updates to imports, type annotations, and usage of external libraries. Key changes include:

  • Migration of MDX-related functionality from the site to a dedicated package (@node-core/mdx).
  • Updates to module imports and type annotations across MDX files.
  • Removal of legacy files and tests no longer required.

Reviewed Changes

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

Show a summary per file
File Description
packages/mdx/lib/shiki.ts Updated type annotations and import paths for Shiki highlighter.
packages/mdx/lib/plugins.ts Added type annotations to plugin exports.
packages/mdx/lib/index.tsx Refactored MDX compilation with strengthened type definitions.
packages/mdx/lib/highlight.ts Updated import path for Shiki configuration.
packages/mdx/lib/evaluate.ts Introduced a new evaluator using Sval.
packages/mdx/eslint.config.js Updated ESLint configuration with new TypeScript rules.
apps/site/util/gitHubUtils.ts Removed unused slugger creation, now provided elsewhere.
apps/site/pages/id/about/security-reporting.mdx Fixed broken link URL formatting.
apps/site/package.json Updated dependencies to reflect usage of the new MDX package.
apps/site/next.dynamic.mjs Updated MDX compiler imports to use the new package.
apps/site/next.config.mjs Added new package dependency for @radix-ui/react-avatar.
apps/site/components/withAvatarGroup.ts Removed unused Image import in favor of streamlined props.
apps/site/components/MDX/Image/index.tsx Removed unnecessary image unoptimized property.
apps/site/components/MDX/CodeBox/index.tsx Updated import for language display utilities.
apps/site/components/Downloads/Release/ReleaseCodeBox.tsx Updated evaluator and highlighter imports to the new MDX package.
Comments suppressed due to low confidence (1)

packages/mdx/lib/index.tsx:88

  • The variable 'slugger' is used without being defined. To resolve this, instantiate it using the imported 'createGitHubSlugger' function (e.g., const slugger = createGitHubSlugger()) before using it.
heading.data = { ...heading.data, id: slugger(heading.value) };

@avivkeller avivkeller changed the title chore: resolve some todos feat(mdx): move mdx into sub-package May 16, 2025
@codecov-commenter
Copy link

codecov-commenter commented May 16, 2025

❌ 2 Tests Failed:

Tests completed Failed Passed Skipped
5 2 3 0
View the top 2 failed test(s) by shortest run time
test::getLanguageDisplayName
Stack Traces | 0.000879s run time
[Error [ERR_TEST_FAILURE]: Cannot find module '.../runner/work/nodejs.org/nodejs..../packages/mdx/shiki.config.mjs' imported from .../runner/work/nodejs.org/nodejs..../lib/__tests__/utils.test.mjs] {
  code: 'ERR_TEST_FAILURE',
  failureType: 'testCodeFailure',
  cause: Error [ERR_MODULE_NOT_FOUND]: Cannot find module '.../runner/work/nodejs.org/nodejs..../packages/mdx/shiki.config.mjs' imported from .../runner/work/nodejs.org/nodejs..../lib/__tests__/utils.test.mjs
      at finalizeResolution (node:.../modules/esm/resolve:275:11)
      at moduleResolve (node:.../modules/esm/resolve:860:10)
      at defaultResolve (node:.../modules/esm/resolve:984:11)
      at nextResolve (node:.../modules/esm/hooks:748:28)
      at resolveBase (file://.../runner/work/nodejs.org/nodejs.org/node_modules/.pnpm/[email protected]..../dist/esm/index.mjs?1747609217982:2:3212)
      at async resolveDirectory (file://.../runner/work/nodejs.org/nodejs.org/node_modules/.pnpm/[email protected]..../dist/esm/index.mjs?1747609217982:2:3578)
      at async resolve (file://.../runner/work/nodejs.org/nodejs.org/node_modules/.pnpm/[email protected]..../dist/esm/index.mjs?1747609217982:2:4441)
      at async nextResolve (node:.../modules/esm/hooks:748:22)
      at async resolve (node:.../test_runner/mock/loader:78:29)
      at async nextResolve (node:.../modules/esm/hooks:748:22) {
    code: 'ERR_MODULE_NOT_FOUND',
    url: 'file://.../runner/work/nodejs.org/nodejs..../packages/mdx/shiki.config.mjs'
  }
}
test::getMetaParameter extracts values from meta strings
Stack Traces | 0.00311s run time
Error [ERR_TEST_FAILURE]: Expected values to be strictly equal:
+ actual - expected

+ undefined
- 'JavaScript'

    at new Promise (<anonymous>)
    at Array.map (<anonymous>) {
  code: 'ERR_TEST_FAILURE',
  failureType: 'testCodeFailure',
  cause: AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
  + actual - expected
  
  + undefined
  - 'JavaScript'
  
      at <anonymous> (.../runner/work/nodejs.org/nodejs..../lib/__tests__/shiki.test.mjs:86:16)
      at Array.forEach (<anonymous>)
      at TestContext.<anonymous> (.../runner/work/nodejs.org/nodejs..../lib/__tests__/shiki.test.mjs:85:13)
      at Test.runInAsyncScope (node:async_hooks:214:14)
      at Test.run (node:internal/test_runner/test:1047:25)
      at Test.start (node:internal/test_runner/test:944:17)
      at node:internal/test_runner/test:1440:71
      at node:internal/per_context/primordials:483:82
      at new Promise (<anonymous>)
      at new SafePromise (node:internal/per_context/primordials:451:29) {
    generatedMessage: true,
    code: 'ERR_ASSERTION',
    actual: undefined,
    expected: 'JavaScript',
    operator: 'strictEqual'
  }
}

To view more test analytics, go to the Test Analytics Dashboard
📋 Got 3 mins? Take this short survey to help us improve Test Analytics.

@avivkeller
Copy link
Member Author

Follow-ups (for separate PRs):

  • Allow rehypeShiki to be configurable enough to be used in api-docs-tooling as the Shiki parser
  • Move CodeTabs and CodeBox into this package

Copy link
Member

@AugustinMauroy AugustinMauroy left a comment

Choose a reason for hiding this comment

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

could you try to add some unit test

@avivkeller
Copy link
Member Author

@AugustinMauroy can you please check if the added tests are to your satisfaction?

Thank you

@ovflowd
Copy link
Member

ovflowd commented May 19, 2025

he MDX functionality can be extracted into its own package, which might also be useful for api-docs-tooling down the line.

I don't think we need MDX functionality on api-docs-tooling itself; Only Shiki, but that is unified and can run directly with rehype/remark. But I'm fine in general moving all the custom stuff we've made for our needs into its own package...

@@ -1,5 +1,7 @@
'use client';

import createSval from '@node-core/mdx/evaluator';
Copy link
Member

Choose a reason for hiding this comment

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

Not sure calling it /mdx makes sense.... I think making it clearer it is web stuff related makes sense? No idea for a name btw

Copy link
Member

Choose a reason for hiding this comment

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

Can you let us know if you changed this code or was it just a 1:1 move? Why did git not understand it as a move?

@@ -0,0 +1,54 @@
import {
Copy link
Member

Choose a reason for hiding this comment

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

Separate type import from regular imports...

});

const match = html.match(/<code[^>]*>([\s\S]*?)<\/code>/);
return match?.[1] || html;
Copy link
Member

Choose a reason for hiding this comment

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

I don't recall this code being like this before? Or was it?

Copy link
Member Author

@avivkeller avivkeller May 19, 2025

Choose a reason for hiding this comment

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

https://github.com/nodejs/nodejs.org/pull/7752/files#diff-0a8ce2f723de39a166ceda0a65a7707123ad07d6c5bee02c0fc3da5f15282848L20

Typescript didn’t like that we were returning a string when it could be undefined, so I changed the structure accordingly. I can revert if you insist.

@@ -0,0 +1,26 @@
'use strict';
Copy link
Member

Choose a reason for hiding this comment

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

No need of use strict on TypeScript files...

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 not sure why you changed some files from .mjs to .ts... Again, we want to simplify these packages to be easily to be imported by other packages without the need of building them or bundling them. Adding TypeScript diminishes that ability for virtually no real reason. These are libraries; Type verification will ultimately be done on the App-side; And if we're really curious if theu're OK, we can add a CI step for tsc to emit warnings on JSdocs.

@avivkeller
Copy link
Member Author

I don't think we need MDX functionality on api-docs-tooling itself; Only Shiki, but that is unified and can run directly with rehype/remark. But I'm fine in general moving all the custom stuff we've made for our needs into its own package...

Yes, I misunderstood earlier, we only need Shiki to be imported to api-docs-tooling.

As for a name, I just took the prefix the files had (next.mdx).

I suppose "markdown-utilities" could also work?

@avivkeller
Copy link
Member Author

avivkeller commented May 19, 2025

Also, on that note, I’m going to convert these back to JS, like you said, and so that we don’t need a typescript loader to use Shiki in api-docs-tooling. Drafting until then.

Sorry for the confusion with the files (if any).

@avivkeller avivkeller marked this pull request as draft May 19, 2025 00:48

import baseConfig from '../../eslint.config.js';

export default [
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
export default [
/*
* @see https://eslint.org/docs/latest/use/configure/
*/
export default [

@AugustinMauroy
Copy link
Member

@AugustinMauroy can you please check if the added tests are to your satisfaction?

Thank you

That's nice !

@avivkeller
Copy link
Member Author

avivkeller commented May 19, 2025

I think I'll make this @node-core/rehype-shiki, where the Shiki used here and in api-docs-tooling will live. The MDX will stay in apps/site. Given that this is a different direction from the initial premise of this PR, I'm closing this, and I'll open a new PR.

@avivkeller avivkeller closed this May 19, 2025
@avivkeller avivkeller deleted the mdx-pkg branch May 19, 2025 14:50
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.

4 participants