feat(packaging): native-Node-resolvable ESM (extensions, type:module, exports) + CI - #2773
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds ESM packaging validation tooling, updates package exports and build scripts across multiple packages, adds a dist/esm package.json writer, and lowers Playwright matrix concurrency. ChangesESM packaging updates
Playwright CI concurrency
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/package.json (1)
14-59: 🗄️ Data Integrity & Integration | 🟡 MinorConfirm ESM-only exports are intentional (no
require/defaultfallback).The
packages/core/package.jsonexports expose onlytypesandimportfor all subpaths, with norequireordefaultconditions and nodist/cjsbuild. Node will throwERR_PACKAGE_PATH_NOT_EXPORTEDforrequire('@cornerstonejs/core')or its subpaths. If this ESM-only migration is deliberate and CJS consumers will use dynamicimport()or a bundler, this is correct. Otherwise, addrequire/defaultconditions pointing to CJS outputs and publish those artifacts to avoid breaking CommonJS users.Relevant config excerpt
// packages/core/package.json (lines 12–59) "exports": { ".": { "types": "./dist/esm/index.d.ts", "import": "./dist/esm/index.js" }, "./utilities": { "types": "./dist/esm/utilities/index.d.ts", "import": "./dist/esm/utilities/index.js" }, "./utilities/*": { "types": "./dist/esm/utilities/*.d.ts", "import": "./dist/esm/utilities/*.js" }, "./constants": { "types": "./dist/esm/constants/index.d.ts", "import": "./dist/esm/constants/index.js" }, "./constants/*": { "types": "./dist/esm/constants/*.d.ts", "import": "./dist/esm/constants/*.js" }, "./enums": { "types": "./dist/esm/enums/index.d.ts", "import": "./dist/esm/enums/index.js" }, "./enums/*": { "types": "./dist/esm/enums/*.d.ts", "import": "./dist/esm/enums/*.js" }, "./loaders": { "types": "./dist/esm/loaders/index.d.ts", "import": "./dist/esm/loaders/index.js" }, "./loaders/*": { "types": "./dist/esm/loaders/*.d.ts", "import": "./dist/esm/loaders/*.js" }, "./types": { "types": "./dist/esm/types/index.d.ts" }, "./types/*": { "types": "./dist/esm/types/*.d.ts" }, "./version": { "types": "./dist/esm/version.d.ts", "import": "./dist/esm/version.js" }, "./package.json": "./package.json" }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/package.json` around lines 14 - 59, The package exports in package.json are ESM-only, so confirm this is intentional and acceptable for consumers of the main entry and subpaths. Review the export map for ".", "./utilities", "./constants", "./enums", "./loaders", "./types", and "./version" and either document that `@cornerstonejs/core` must be consumed via import/dynamic import only, or add require/default conditions plus the corresponding CJS build outputs if CommonJS support is still needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/adapters/package.json`:
- Around line 15-55: The package exports are ESM-only because the exports map in
package.json entries like "./cornerstone", "./cornerstone3D", "./enums",
"./helpers", and "./version" only provide types/import paths with no CommonJS
fallback. Confirm this is an intentional breaking change for consumers of the
adapters package; if CJS support is still required, update the exports map to
include a require condition (or default fallback) pointing to the CommonJS
build, otherwise keep it as-is and ensure the release notes/major version
reflect the removal of require() support.
---
Outside diff comments:
In `@packages/core/package.json`:
- Around line 14-59: The package exports in package.json are ESM-only, so
confirm this is intentional and acceptable for consumers of the main entry and
subpaths. Review the export map for ".", "./utilities", "./constants",
"./enums", "./loaders", "./types", and "./version" and either document that
`@cornerstonejs/core` must be consumed via import/dynamic import only, or add
require/default conditions plus the corresponding CJS build outputs if CommonJS
support is still needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c05e10cd-2342-4ac6-a115-b02fecba6737
📒 Files selected for processing (12)
packages/adapters/package.jsonpackages/adapters/rollup.config.mjspackages/ai/package.jsonpackages/core/package.jsonpackages/dicomImageLoader/package.jsonpackages/labelmap-interpolation/package.jsonpackages/metadata/package.jsonpackages/nifti-volume-loader/package.jsonpackages/polymorphic-segmentation/package.jsonpackages/tools/package.jsonpackages/utils/package.jsonscripts/write-dist-esm-package-json.js
… resolution The published packages emit ESM into dist/esm but never declare it, so Node classifies the output as CommonJS. Node 22-24 tolerate this via syntax detection (at a small perf cost), but Node >=25 hard-fails at link time, and publint / @arethetypeswrong report FILE_INVALID_FORMAT. See #2763. Make the packaging metadata correct and self-describing for every published package (core, tools, dicom-image-loader, nifti-volume-loader, polymorphic-segmentation, ai, labelmap-interpolation, adapters, metadata, utils): - Emit a scoped dist/esm/package.json with {"type":"module"} as a postbuild step (scripts/write-dist-esm-package-json.js). This signals ESM for the published output without adding top-level "type":"module", which would force ESM semantics onto each package's root-level CommonJS config files (.webpack/*.js, babel.config.js, karma.conf.js). - Rewrite every exports map: "types" first, then "import", then a "default" condition pointing at the same ESM file. The "default" replaces the uneven standalone "node" condition and keeps require(esm) resolving on Node >=22.12 (an exports map with only "import" makes require() throw ERR_PACKAGE_PATH_NOT_EXPORTED), so this stays non-breaking. Also add "./package.json": "./package.json". - adapters builds with rollup preserveModules from src/index.ts, so version.js was never emitted (nothing imports it) even though exports["./version"] points at it. Add src/version.ts as an explicit rollup input so the file is emitted. This is the non-breaking packaging-metadata step. Rewriting the still-extensionless relative import specifiers to explicit .js extensions (by moving the library build onto a bundler) is the follow-up needed for a full native-Node import() of the package entry points. Refs #2763
sedghi
force-pushed
the
cornerstoneNodeCompat
branch
from
June 26, 2026 15:59
4b50e2b to
3693c53
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/dicomImageLoader/package.json`:
- Around line 29-75: The package exports are now ESM-only in the package.json
exports map, so this is a breaking change for CommonJS consumers. Make sure the
package metadata and release notes explicitly document that require() is no
longer supported, and confirm the exports entries such as "./imageLoader",
"./wadors", and "./wadouri" intentionally resolve only to the ESM dist paths
with no require condition or CJS fallback.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 257818ea-3e9e-441d-ad35-367a85b35eca
📒 Files selected for processing (12)
packages/adapters/package.jsonpackages/adapters/rollup.config.mjspackages/ai/package.jsonpackages/core/package.jsonpackages/dicomImageLoader/package.jsonpackages/labelmap-interpolation/package.jsonpackages/metadata/package.jsonpackages/nifti-volume-loader/package.jsonpackages/polymorphic-segmentation/package.jsonpackages/tools/package.jsonpackages/utils/package.jsonscripts/write-dist-esm-package-json.js
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/adapters/rollup.config.mjs
- packages/ai/package.json
- packages/labelmap-interpolation/package.json
- packages/core/package.json
- packages/utils/package.json
- packages/adapters/package.json
- packages/tools/package.json
- packages/metadata/package.json
- packages/polymorphic-segmentation/package.json
…ner count Only two runners carry the self-hosted label, so the matrix never ran more than two legs concurrently anyway; max-parallel: 3 left the third leg queued behind a slot that can never be filled. Align the declared limit with reality.
Phase B of the ESM packaging work (follows the type:module + exports cleanup). The published packages emitted ESM with extensionless relative imports, which bundlers tolerate but native Node ESM rejects (Node >=25 hard-fails). See #2763. - Build: run `tsc-alias --resolve-full-paths` after tsc (and after rollup for adapters) so emitted .js AND .d.ts relative imports carry explicit .js extensions (directory imports -> /index.js). Source stays extensionless; this is a dist-only post-build rewrite, not a source codemod. - Tests: add scripts/validate-esm-packaging.mjs (pnpm run validate:packaging). Per published package it checks the dist/esm type:module marker, that every relative import in .js/.d.ts resolves to a real file, that relative `new URL(...)` assets exist (e.g. the dicom-image-loader decode worker), publint (errors), attw (esm-only profile), and a native-Node leaf import. - CI: add .github/workflows/validate-packaging.yml (PR + dispatch) that builds and validates on Node 24 and 25, so this cannot regress. - Docs: note the native-Node ESM change and that legacy moduleResolution:node (node10) consumers should move to bundler/node16/nodenext. Adds tsc-alias, publint and @arethetypeswrong/cli as dev dependencies; the large pnpm-lock.yaml diff is mostly a pnpm 11.5.2 reserialization of the previously stale-format lockfile (no dependency version changes). Refs #2763
sedghi
changed the title
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/validate-packaging.yml:
- Around line 14-16: Add an explicit least-privilege permissions block to the
validate-packaging job so it does not inherit the default broad GITHUB_TOKEN
scopes; in the validate-packaging workflow, update the job definition to set
only the minimal read-only permissions needed for checkout/build/validation. Use
the validate-packaging job as the anchor for the change and keep the permissions
scoped tightly since this workflow performs no write operations.
- Around line 22-29: Add `persist-credentials: false` to the Checkout code step
that uses actions/checkout@v6.0.3 in the validate-packaging workflow so the
GitHub token is not left in local git config; update the checkout action’s with
block in that job while leaving the pnpm/action-setup and actions/setup-node
steps unchanged.
In `@scripts/validate-esm-packaging.mjs`:
- Around line 32-51: The listPublishedPackages helper currently assumes every
lerna.json package entry is a literal directory, which could silently skip
packages if a glob pattern is introduced later. Update listPublishedPackages in
scripts/validate-esm-packaging.mjs to explicitly validate each lerna.packages
entry before iterating, and throw if any entry matches a glob pattern instead of
being a direct directory path. Keep the existing package discovery logic for
package.json and dist/esm, but make the glob guard part of the early validation
so future lerna.json format changes fail fast rather than reducing coverage.
- Around line 158-178: The `checkPublint` helper is importing `formatMessage`
from the wrong module path, so it stays unavailable and the formatted error
output never works. Update the `checkPublint` import logic to load
`formatMessage` from the `publint/utils` subpath while keeping the existing
fallback behavior, and leave the `publint` root import only for the main API
used to collect messages. Ensure the `formatMessage` reference in `checkPublint`
continues to be used for human-readable error strings before falling back to raw
codes/args.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6c6e34a0-1059-456f-90ef-b6d6302e5bb7
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (14)
.github/workflows/validate-packaging.ymlpackage.jsonpackages/adapters/package.jsonpackages/ai/package.jsonpackages/core/package.jsonpackages/dicomImageLoader/package.jsonpackages/docs/docs/migration-guides/5x/1-migration-notes.mdpackages/labelmap-interpolation/package.jsonpackages/metadata/package.jsonpackages/nifti-volume-loader/package.jsonpackages/polymorphic-segmentation/package.jsonpackages/tools/package.jsonpackages/utils/package.jsonscripts/validate-esm-packaging.mjs
✅ Files skipped from review due to trivial changes (1)
- packages/docs/docs/migration-guides/5x/1-migration-notes.md
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/nifti-volume-loader/package.json
- packages/core/package.json
- packages/tools/package.json
- packages/labelmap-interpolation/package.json
- packages/ai/package.json
- packages/utils/package.json
- packages/polymorphic-segmentation/package.json
- packages/metadata/package.json
- packages/dicomImageLoader/package.json
# Conflicts: # pnpm-lock.yaml
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/playwright.yml (1)
19-19: 🧹 Nitpick | 🔵 TrivialReduced concurrency will slow overall Playwright CI runtime.
Dropping
max-parallelfrom 3 to 2 forces one of the three matrix jobs (legacy/compatibility/next) to queue, increasing total pipeline duration. This is likely intentional to reduce load on the self-hostednashuarunner pool, but worth confirming that's the goal versus an oversight.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/playwright.yml at line 19, The Playwright workflow matrix is throttled by reducing max-parallel, which increases CI runtime; review the concurrency setting in the workflow job and either restore the previous level or keep the lower limit only if reducing load on the self-hosted nashua runners is intentional. Update the matrix configuration around max-parallel in the Playwright workflow so it matches the desired balance between throughput and runner capacity.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.github/workflows/playwright.yml:
- Line 19: The Playwright workflow matrix is throttled by reducing max-parallel,
which increases CI runtime; review the concurrency setting in the workflow job
and either restore the previous level or keep the lower limit only if reducing
load on the self-hosted nashua runners is intentional. Update the matrix
configuration around max-parallel in the Playwright workflow so it matches the
desired balance between throughput and runner capacity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b15897a1-e798-4840-a6e8-eadc1d8f600c
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (11)
.github/workflows/playwright.ymlpackages/adapters/package.jsonpackages/ai/package.jsonpackages/core/package.jsonpackages/dicomImageLoader/package.jsonpackages/labelmap-interpolation/package.jsonpackages/metadata/package.jsonpackages/nifti-volume-loader/package.jsonpackages/polymorphic-segmentation/package.jsonpackages/tools/package.jsonpackages/utils/package.json
🚧 Files skipped from review as they are similar to previous changes (10)
- packages/ai/package.json
- packages/labelmap-interpolation/package.json
- packages/nifti-volume-loader/package.json
- packages/dicomImageLoader/package.json
- packages/tools/package.json
- packages/polymorphic-segmentation/package.json
- packages/core/package.json
- packages/adapters/package.json
- packages/utils/package.json
- packages/metadata/package.json
jbocce
reviewed
Jul 3, 2026
| "webpack:watch": "pnpm -r --filter=!docs run webpack:watch" | ||
| }, | ||
| "devDependencies": { | ||
| "@arethetypeswrong/cli": "^0.18.4", |
jbocce
reviewed
Jul 3, 2026
| "postcss-loader": "7.3.4", | ||
| "postcss-preset-env": "8.5.1", | ||
| "prettier": "3.6.2", | ||
| "publint": "^0.3.21", |
jbocce
reviewed
Jul 3, 2026
| "stylelint-config-recommended": "12.0.0", | ||
| "ts-loader": "9.5.1", | ||
| "ts-node": "10.9.2", | ||
| "tsc-alias": "^1.8.17", |
jbocce
reviewed
Jul 3, 2026
| // from ever regressing. Run AFTER building (dist/esm must exist). | ||
| // | ||
| // Per package it checks: | ||
| // 1. ESM marker - dist/esm/package.json declares {"type":"module"} |
jbocce
requested changes
Jul 3, 2026
jbocce
left a comment
jbocce
left a comment
Collaborator
There was a problem hiding this comment.
Please use exact versioning in the package.json files.
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
jbocce
reviewed
Jul 4, 2026
| // Absorb sub-pixel font drift on the "138 mm" label across CI | ||
| // environments; a missing/mis-drawn annotation differs by far more. | ||
| { maxDiffPixelRatio: 0.003 } | ||
| { maxDiffPixelRatio: 0.003, timeoutMs: 20000 } |
Collaborator
There was a problem hiding this comment.
I am surprised you needed this. In general I have found that the timeout for visitExample needs to be increased for these types of tests. FYI.
jbocce
approved these changes
Jul 4, 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
3 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.
Summary
The published
@cornerstonejs/*packages emit ESM intodist/esm/**but didn't fully declare it: notype:module, slightly inconsistentexports, and extensionless relative imports. Bundlers (webpack/Vite/Next — OHIF and most viewers) tolerate all of this, but a consumer loading the files on a native Node code path (SSR, server, Node test runners, packaging linters) cannot resolve them — Node 22–24 limp along via syntax detection, Node ≥25 hard-fails at link time, andpublint/@arethetypeswrongflagFILE_INVALID_FORMAT. Reported in #2763.This PR makes the published packages correct and self-describing for both bundlers and native Node, and adds CI so it can't regress. It is non-breaking (the files were already ESM; this makes Node classify and resolve them correctly) and applies to every published package:
core,tools,dicom-image-loader,nifti-volume-loader,polymorphic-segmentation,ai,labelmap-interpolation,adapters,metadata,utils.Changes
Packaging metadata
dist/esm/package.json={"type":"module"}(postbuild step), so Node treats the output as ESM without forcing ESM onto each package's root-level CommonJS config files (.webpack/*.js,babel.config.js,karma.conf.js).exportsmap:typesfirst, thenimport/default; drop the redundant standalonenodecondition; add"./package.json": "./package.json".adapters: RolluppreserveModulesnever emittedversion.js(not reachable from the entry) thoughexports["./version"]pointed at it —src/version.tsis now an explicit Rollup input.Real extensions (native-Node resolution)
tsc-alias --resolve-full-pathsaftertsc(and afterrollupforadapters) so emitted relative imports in both.jsand.d.tscarry explicit.jsextensions (directory imports →/index.js). Source stays extensionless — this is a dist-only post-build rewrite, not a source codemod.dicom-image-loaderdecode worker (new Worker(new URL('./decodeImageFrameWorker.js', import.meta.url))) and the codecnew URL(..., import.meta.url)references are emitted verbatim, and the worker file ships as a real sibling, so consumer bundlers continue to handle them.Tests + CI (so this can't regress)
scripts/validate-esm-packaging.mjs(pnpm run validate:packaging): per package, verifies thetype:modulemarker, that every relative import in.js/.d.tsresolves to a real file, that relativenew URL(...)assets exist, then runspublint,@arethetypeswrong/cli(esm-onlyprofile), and a native-Node leaf import. Negative-tested: it fails (exit 1) on a reintroduced extensionless import..github/workflows/validate-packaging.yml: builds and validates on every PR across Node 24 and 25 (25 is where ESM-only packages ship dist/esm/*.js without "type": "module" (no root field, no nested package.json) → Node resolves output as CJS #2763 hard-fails).Validation
publintis clean andattwreportsnode16 (from ESM): 🟢for all 10 packages; the full validator passes for all 10 from a cleanrm dist → build → validate.Notes for release
exportssurface change; bundler consumers resolve identically. Lands as afeat:(semantic-release cuts the minor).moduleResolutioncaveat: types now use.js-extensioned ESM imports. Consumers on the legacymoduleResolution: "node"(node10) — which can't map a.jsspecifier to its.d.tsand ignoresexports— may see some deep types resolve asany; switch to"bundler","node16", or"nodenext". Type-only, no runtime impact. Documented inmigration-guides/5x.pnpm-lock.yamldiff is mostly a pnpm 11.5.2 reserialization of the previously stale-format lockfile (no dependency version changes) plus the three new dev dependencies (tsc-alias,publint,@arethetypeswrong/cli).Out of scope (separate follow-ups)
Removing the orphan UMD bundles (a breaking change → the eventual major), lazy-loading vtk.js, and DOM-access guards so the heavy package entries (
core/tools) also execute in DOM-less Node. Native-Node WASM decode indicom-image-loaderadditionally needs a small source change (bare-specifiernew URL→import.meta.resolve) and is independent of this packaging work.Refs #2763
Summary by CodeRabbit
dist/esm/package.jsonmarker (type: "module"), and multiple packages expose./package.jsonpublicly.defaultalongsideimportand type definitions../versionsubpath ESM resolution by adjusting ESM build outputs and export conditions.validate:packagingscript to verify ESM correctness.