render-helper: make the node build loadable by plain Node - #1566
Merged
Conversation
remarkable/linkify is a legacy module directory: a folder holding nothing but a package.json stub with main and module, no index.js. CJS resolves that stub, Node's ESM resolver refuses it, so the emitted dist/node/index.mjs could not be imported by plain Node at all. Every bundler resolves the directory happily, which is why nothing here caught it. The newsletter service hit it in production and has been loading the package through createRequire since. remarkable is inlined into the node build now, and only that build. The shared external list is untouched, so dist/browser/index.js comes out byte for byte what it is today, which matters because that is the file React Native loads through the react-native export condition. Not the smaller-looking fix of importing a file inside remarkable: remarkable/dist/esm/linkify.js is a .js in a package with no type module, so it is nominally CommonJS and only loads on Node 22.7 and later through module-syntax detection, while this build targets node18. The .cjs file does work everywhere but drags autolinker's CommonJS build into every browser bundle in place of its tree-shakeable one, which is about 5.4KB gzipped on ecency.com and on every self-hosted blog. Inlining costs 237KB in a build only servers and CLIs load. The test spawns the real node binary against real build output, for both node conditions, and checks the module loads, renders and autolinks. Running it inside vitest would prove nothing, since Vite is the layer that hid this. It builds into a scratch directory rather than reading dist, because dist is committed and the changeset bot is what rebuilds it, so on this branch the committed copy is legitimately stale and asserting against it would go red on a fresh checkout and on web-build.yml, which runs the tests with no preceding package build.
Contributor
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Summary by QodoMake render-helper Node bundles loadable without a bundler
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Greptile SummaryThis PR makes the render-helper Node distribution directly loadable through both ESM and CommonJS by bundling Remarkable only into the Node output.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| packages/render-helper/tsup.config.ts | Bundles Remarkable exclusively into the Node ESM and CommonJS outputs while leaving the browser dependency policy unchanged. |
| packages/render-helper/test/dist-loads-in-plain-node.spec.ts | Builds isolated output and validates direct Node loading, rendering, dependency inlining, license notices, exports, and metafile cleanup. |
| packages/render-helper/scripts/third-party-notices.mjs | Derives bundled dependency notices from tsup metafiles and removes those build-only metadata files. |
| packages/render-helper/package.json | Updates the package version and makes the build clean output once before generating bundles and third-party notices. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
S[src/index.ts] --> T[tsup]
T --> B[Browser ESM output]
T --> N[Node ESM and CJS outputs]
N --> R[Remarkable bundled]
T --> M[Metafiles]
M --> L[Third-party notices]
N --> P[Plain Node import and require tests]
Reviews (4): Last reviewed commit: "chore: apply changeset versioning for PR..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Bundling remarkable pulls autolinker and tslib in with it. Remarkable and autolinker are MIT, which requires the copyright and permission notice to travel with copies of the software, and the emitted bundles carried no notice of any kind: no @license, no @preserve, no copyright line. That is a licence problem, not a style one. The build generates dist/THIRD-PARTY-NOTICES.md from esbuild's own metafile, so the list is whatever the bundler actually inlined rather than a hand-written one that drifts. A hand-written list would already have been wrong: remarkable also depends on argparse, which is tree-shaken out and must not be claimed as bundled. The metafiles are consumed and deleted so they never reach the tarball, and npm pack now carries the notices with the full MIT text and both copyright lines. The browser build no longer cleans. Cleaning from inside one of two configs only ever cleaned that config's own directory, left dist/node to accumulate stale files, and raced with the other build as soon as both were pointed at a single directory, which is what a test that overrides the output directory does. The build script clears dist once, before either build runs. Review round on the test itself: - renamed to .spec.ts, which is what the other twenty tests in this package use. - the entry path travels in the environment instead of being interpolated into the snippet, so no code is constructed from a value. - both subprocess cases carry an explicit timeout, rather than a sixty second subprocess sitting under vitest's five second default. - the parsed subprocess output and the parsed manifest are typed. - added: the notices must exist and cover every inlined package, dist must be what npm packs, and no metafile may survive into the output.
Both were 60 seconds, so a loaded worker would reach vitest's deadline at the same moment execFile tried to kill the child, and the failure would be a bare outer timeout rather than the subprocess error saying what went wrong. The child gets 30 seconds, the case three times that.
This was referenced Aug 21, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1519.
remarkable/linkifyis a legacy module directory: a folder holding nothing but apackage.jsonstub withmain/module, noindex.js. CJS resolves that stub, Node's ESM resolver refuses it, so the emitteddist/node/index.mjscould not be loaded by plain Node at all:Reproduced on Node 24 and Node 18, the build's own target. Every bundler resolves the directory happily, which is why nothing here caught it: the failure only exists where there is no bundler. The newsletter service hit it in production and has been carrying a
createRequireworkaround since.The change
One option, on the node build only:
The shared
externallist is untouched, so the browser build is byte-identical. That matters more than it sounds: React Native resolves thereact-nativeexport condition todist/browser/index.js, so the file the mobile app loads is unchanged by this PR, today and after any future version bump.Why not the obvious one-line import change
Pointing the source at a file inside remarkable looks cheaper and is not:
remarkable/dist/esm/linkify.jsis a.jsin a package with no"type": "module", so it is nominally CommonJS. It only loads on Node >= 22.7 via unflagged module-syntax detection, and fails outright on Node 18 and 20. The package declares noenginesand is published publicly.remarkable/dist/cjs/linkify.jsdoes work everywhere, but it drags autolinker's CommonJS build into every browser bundle instead of its tree-shakeable one, about +5.4 KB gzipped on ecency.com and on every self-hosted blog.Inlining costs ~237 KB in a build that only servers and CLIs load, and nothing anywhere else.
The test
test/dist-loads-in-plain-node.test.tsspawns the realnodebinary against the real build output, for both theimportandrequireconditions, and asserts the module loads, renders, and autolinks. Importing from inside vitest would prove nothing, since Vite is exactly the layer that hid this.It runs in CI:
PR builddoespnpm build:packagesand thenpnpm -r test, in that order.Negative-controlled. With
noExternalremoved and the package rebuilt:Verification
Three Fable agents were pointed at this with instructions to break it, not to bless it.
Mobile.
dist/browser/index.jsis byte-identical by sha256 (8eb87e6c…) across the committed file, two rebuilds, and the npm-published 2.5.26. The real metro-resolver 0.82.5, driven with vision-mobile's actual config, resolves@ecency/render-helperto that file, so the fix contributes zero bytes to what React Native loads. A future version bump delivers exactly what npm latest already delivers. vision-mobile's own suite: 896 tests pass.Forcing
unstable_enablePackageExports: false, the fallback path, resolves todist/node/index.cjs, which is the file that changed. It still works, and it got safer: the pre-fix fallback pulled remarkable's browser variant, which decodes entities through a DOM<textarea>, while the inlined copy is the DOM-free node variant.Output. A 74-input corpus across 8 call shapes (
renderPostBodyboth modes,postBodySummaryat three lengths,simpleMarkdownToHTML,catchPostImage) is byte-identical before and after, with the autolink comparison confirmed non-vacuous.Consumers. The newsletter service's plain
importnow works against a packed tarball of this build, and its existingcreateRequirepath still works, so removing the workaround is optional rather than forced.hosting/apiunaffected. apps/web builds (Next 15 production) and its suite passes, with no duplicate remarkable in.next. apps/self-hosted builds, its 1004 tests pass, and the node-globals guard is clean.They also found three real holes in the test, which are fixed in this branch:
dist, which is deliberately stale here, so it went red on a fresh checkout and would have gone red onweb-build.yml, which runspnpm -r testwith no precedingpnpm build:packages. It builds its own output now.import "remarkable/linkify", which is the exact shape the issue describes. All six emission forms are covered now, verified one by one, and it still ignores the deprecation string inside the inlined remarkable that readsimport linkify from 'remarkable/linkify'./tmpfailed withERR_MODULE_NOT_FOUNDon the first external, since Node resolves bare specifiers by walking up from the importing file. It builds inside the package tree.Local: render-helper 1256 tests, typecheck and lint clean; self-hosted 1004 tests and a clean build; apps/web builds.
Note for the reviewer
packages/render-helper/distis deliberately left at its committed state. Thepatch:render-helperlabel is what makes auto-changeset bump the version, rebuild the package and commitdistback onto this branch. Without that label the source fix reaches nobody, because both apps consume the package throughworkspace:*, which means through the committeddist.Nothing needs to change in vision-mobile for this. It is unaffected by the bug and unaffected by the fix.