Skip to content

chore(remark): reduce drilling#729

Open
avivkeller wants to merge 2 commits intomainfrom
lazy-remark
Open

chore(remark): reduce drilling#729
avivkeller wants to merge 2 commits intomainfrom
lazy-remark

Conversation

@avivkeller
Copy link
Copy Markdown
Member

@avivkeller avivkeller commented Mar 30, 2026

Changes the loading of remark to be a lazy-loaded function, rather than passed down the chain of function to function

There's a few benefits:

In some generators, we loading remark on both the parent and child thread, when it's only used on the child thread.
In cases where multiple generators are using the same remark, currently, that remark will be re-loaded for each generator
It makes our function signatures smaller, and therefore more readable

@vercel
Copy link
Copy Markdown

vercel bot commented Mar 30, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
api-docs-tooling Ready Ready Preview Mar 30, 2026 4:00pm

Request Review

@avivkeller avivkeller marked this pull request as ready for review March 30, 2026 14:51
@avivkeller avivkeller requested a review from a team as a code owner March 30, 2026 14:51
Copilot AI review requested due to automatic review settings March 30, 2026 14:51
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 30, 2026

Codecov Report

❌ Patch coverage is 70.52632% with 28 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.42%. Comparing base (27de0e7) to head (b9bb60e).

Files with missing lines Patch % Lines
src/generators/legacy-html/utils/buildContent.mjs 0.00% 9 Missing ⚠️
src/generators/jsx-ast/utils/buildContent.mjs 56.25% 7 Missing ⚠️
src/generators/legacy-html/generate.mjs 0.00% 4 Missing ⚠️
src/utils/remark.mjs 76.92% 3 Missing ⚠️
src/generators/legacy-html-all/generate.mjs 0.00% 2 Missing ⚠️
src/generators/legacy-json/utils/buildSection.mjs 33.33% 2 Missing ⚠️
src/generators/jsx-ast/utils/signature.mjs 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #729      +/-   ##
==========================================
+ Coverage   76.37%   76.42%   +0.04%     
==========================================
  Files         155      155              
  Lines       13766    13734      -32     
  Branches     1093     1095       +2     
==========================================
- Hits        10514    10496      -18     
+ Misses       3247     3233      -14     
  Partials        5        5              

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

@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 30, 2026

api-links Generator

apilinks.json
Expected values to be strictly deep-equal:
+ actual - expected
... Skipped lines

  {
    'Agent.defaultMaxSockets': 'https://github.com/nodejs/node/blob/HEAD/lib/_http_agent.js#L227',
    'Buffer.alloc': 'https://github.com/nodejs/node/blob/HEAD/lib/buffer.js#L436',
    'Buffer.allocUnsafe': 'https://github.com/nodejs/node/blob/HEAD/lib/buffer.js#L450',
    'Buffer.allocUnsafeSlow': 'https://github.com/nodejs/node/blob/HEAD/lib/buffer.js#L462',
...
    'agent.addRequest': 'https://github.com/nodejs/node/blob/HEAD/lib/_http_agent.js#L292',
+   'agent.createConnection': 'https://github.com/nodejs/node/blob/HEAD/lib/https.js#L326',
-   'agent.createConnection': 'https://github.com/nodejs/node/blob/HEAD/lib/_http_agent.js#L231',
    'agent.createSocket': 'https://github.com/nodejs/node/blob/HEAD/lib/_http_agent.js#L363',
    'agent.destroy': 'https://github.com/nodejs/node/blob/HEAD/lib/_http_agent.js#L595',
+   'agent.getName': 'https://github.com/nodejs/node/blob/HEAD/lib/https.js#L484',
-   'agent.getName': 'https://github.com/nodejs/node/blob/HEAD/lib/_http_agent.js#L261',
    'agent.keepSocketAlive': 'https://github.com/nodejs/node/blob/HEAD/lib/_http_agent.js#L552',
    'agent.removeSocket': 'https://github.com/nodejs/node/blob/HEAD/lib/_http_agent.js#L491',
    'agent.reuseSocket': 'https://github.com/nodejs/node/blob/HEAD/lib/_http_agent.js#L588',
    'assert.assert': 'https://github.com/nodejs/node/blob/HEAD/lib/assert.js#L185',
    'asyncResource.asyncId': 'https://github.com/nodejs/node/blob/HEAD/lib/async_hooks.js#L242',
...
    'server.address': 'https://github.com/nodejs/node/blob/HEAD/lib/net.js#L2289',
+   'server.close': 'https://github.com/nodejs/node/blob/HEAD/lib/net.js#L2422',
+   'server.closeAllConnections': 'https://github.com/nodejs/node/blob/HEAD/lib/https.js#L120',
+   'server.closeIdleConnections': 'https://github.com/nodejs/node/blob/HEAD/lib/https.js#L122',
-   'server.close': 'https://github.com/nodejs/node/blob/HEAD/lib/_http_server.js#L614',
-   'server.closeAllConnections': 'https://github.com/nodejs/node/blob/HEAD/lib/_http_server.js#L624',
-   'server.closeIdleConnections': 'https://github.com/nodejs/node/blob/HEAD/lib/_http_server.js#L636',
    'server.getConnections': 'https://github.com/nodejs/node/blob/HEAD/lib/net.js#L2384',
    'server.listen': 'https://github.com/nodejs/node/blob/HEAD/lib/net.js#L2106',
    'server.ref': 'https://github.com/nodejs/node/blob/HEAD/lib/net.js#L2527',
+   'server.setTimeout': 'https://github.com/nodejs/node/blob/HEAD/lib/https.js#L124',
-   'server.setTimeout': 'https://github.com/nodejs/node/blob/HEAD/lib/_http_server.js#L652',
    'server.unref': 'https://github.com/nodejs/node/blob/HEAD/lib/net.js#L2536',
+   'server[SymbolAsyncDispose]': 'https://github.com/nodejs/node/blob/HEAD/lib/net.js#L2462',
+   'server[undefined]': 'https://github.com/nodejs/node/blob/HEAD/lib/net.js#L2491',
-   'server[SymbolAsyncDispose]': 'https://github.com/nodejs/node/blob/HEAD/lib/_http_server.js#L620',
-   'server[undefined]': 'https://github.com/nodejs/node/blob/HEAD/lib/_http_server.js#L659',
    'serverresponse._finish': 'https://github.com/nodejs/node/blob/HEAD/lib/_http_server.js#L246',
    'serverresponse._implicitHeader': 'https://github.com/nodejs/node/blob/HEAD/lib/_http_server.js#L360',
    'serverresponse.assignSocket': 'https://github.com/nodejs/node/blob/HEAD/lib/_http_server.js#L296',
    'serverresponse.detachSocket': 'https://github.com/nodejs/node/blob/HEAD/lib/_http_server.js#L307',
    'serverresponse.statusCode': 'https://github.com/nodejs/node/blob/HEAD/lib/_http_server.js#L269',

Copy link
Copy Markdown
Contributor

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 how remark/unified processors are accessed by turning the getRemark* exports into memoized lazy factories and updating downstream code to call them directly instead of threading processor instances through multiple function layers.

Changes:

  • Convert getRemark, getRemarkRehype, getRemarkRehypeWithShiki, and getRemarkRecma into lazy()-memoized factories.
  • Update generators/utilities to call remark() (aliased getRemark*) at point-of-use rather than passing processors through function parameters.
  • Update JSX AST utility tests to mock the remark module via node:test module mocking.

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/utils/remark.mjs Wrap remark processor constructors in lazy() for memoized access.
src/generators/metadata/utils/visitors.mjs Stop lazily wrapping getRemark; call memoized getRemark directly.
src/generators/metadata/utils/parse.mjs Replace per-module processor instance with remark().runSync(...) calls.
src/generators/legacy-json/utils/buildSection.mjs Remove stored processor instance; call remark() when rendering HTML.
src/generators/legacy-html/utils/buildContent.mjs Remove threaded processor param; use imported remark() inside helpers.
src/generators/legacy-html/generate.mjs Remove eager processor instance; use remark() for ToC generation and drop param passing.
src/generators/legacy-html-all/generate.mjs Remove eager processor instance; use remark() for navigation generation.
src/generators/jsx-ast/utils/types.mjs Stop requiring a processor param; import and use getRemarkRecma factory.
src/generators/jsx-ast/utils/signature.mjs Stop passing remark into signature table creation.
src/generators/jsx-ast/utils/buildContent.mjs Stop threading remark through content pipeline; use imported remark() factory.
src/generators/jsx-ast/utils/tests/types.test.mjs Switch tests to module-mock remark.mjs instead of passing a fake processor.
src/generators/jsx-ast/utils/tests/buildContent.test.mjs Update calls to new transformHeadingNode signature (no remark arg).
src/generators/ast/generate.mjs Use remark().parse(...) instead of a pre-created processor instance.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@ovflowd
Copy link
Copy Markdown
Member

ovflowd commented Mar 30, 2026

Changes the loading of remark to be a lazy-loaded function, rather than passed down the chain of function to function

What are the benefits here?

@avivkeller
Copy link
Copy Markdown
Member Author

There's a few benefits:

  1. In some generators, we loading remark on both the parent and child thread, when it's only used on the child thread.
  2. In cases where multiple generators are using the same remark, currently, that remark will be re-loaded for each generator
  3. It makes our function signatures smaller, and therefore more readable

@ovflowd
Copy link
Copy Markdown
Member

ovflowd commented Mar 30, 2026

There's a few benefits:

  1. In some generators, we loading remark on both the parent and child thread, when it's only used on the child thread.
  2. In cases where multiple generators are using the same remark, currently, that remark will be re-loaded for each generator
  3. It makes our function signatures smaller, and therefore more readable

Next time, could you write that on the PR description? Makes it easier for me as a reviewer to understand the trade-offs and motivation behind the change 🙇

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

SGTM!

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

LGMT ! did you have/know if there are perf impact (I guess it's a win)

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