Repository navigation
Make the ESM build resolvable in Node, and run the checks in CI - #24
Conversation
lib/esm/index.js shipped `export * from "./entity"`, which Node's ESM resolver rejects because it needs the full specifier. Importing the package from plain Node failed with ERR_MODULE_NOT_FOUND. Bundlers hid it, since they do their own resolution, so only unbundled consumers saw it: a Node build step, a CLI, a test runner on native ESM, a server. scripts/finalize-esm.mjs now runs after tsc and rewrites the emitted relative specifiers, resolving each one against a file that was actually emitted and failing the build if any of them resolves to nothing. The sources keep their extensionless imports, so jest and the two webpack configs are untouched. It also writes the type markers for both halves. Without them Node classifies lib/esm by the root package.json, which declares no type, and the ESM half loaded only because Node 22 sniffs module syntax. Older runtimes failed on the first `export` instead. scripts/check-esm.mjs imports the built entry from a real Node ESM context and runs a parse through it, and it is wired into the pre-commit chain after build rather than left as something to remember. Fixes #20
| // type. The ESM half then loads only because Node 22 sniffs module syntax, and older runtimes | ||
| // fail on the first `export` instead. | ||
| writeFileSync(join(ESM_DIR, "package.json"), '{\n "type": "module"\n}\n'); | ||
| writeFileSync(join(CJS_DIR, "package.json"), '{\n "type": "commonjs"\n}\n'); |
There was a problem hiding this comment.
This one is not fixing a live bug, so it is fair to ask why it is here. lib/cjs is CommonJS today only because the root package.json declares no type, which makes commonjs the default. Adding "type": "module" at the root later, for any unrelated reason, would silently reclassify every file in lib/cjs and break require("notations"). Pinning both halves means neither depends on the root staying silent.
The repo had no workflows, so build, lint and test ran only in the pre-commit hook, on the machine of whoever was committing, and only if pre-commit was installed there. A commit made with --no-verify, or a PR from a fresh clone, landed with nothing checked. Both bugs fixed in #23 shipped in v1.0.9 and were found downstream rather than here. The matrix exists for check:esm. Node classifies lib/esm from its own package.json marker on every version, but only 22.12 and later fall back to sniffing module syntax, so a regression in that marker surfaces as a different error either side of that line. Install runs with --ignore-scripts so the root prepare script does not build during install, which keeps a broken build reporting as a failed build step rather than a failed install. Fixes #25
|
|
||
| - uses: pnpm/action-setup@v4 | ||
| with: | ||
| version: 12 |
There was a problem hiding this comment.
This reads as a mismatch, since pnpm-lock.yaml says lockfileVersion: '9.0'. It is not one. pnpm 9 through 12 all read and write that lockfile format, and 12 is what is installed on the dev machine this was verified on, so pinning the major here keeps CI and local on the same resolver. Dropping to version: 10would also work. What would not work isnpm ci, since package-lock.json` is gitignored.
What changes
import * as N from "notations"works from plain Node now. The build rewrites the relative specifiers inlib/esmso they carry.js, and writes thetypemarkers that tell Node which half oflib/is which. Sources keep their extensionless imports, so jest and the two webpack configs are untouched. Therequire()path was already fine and stays fine.The same PR adds the repo's first workflow file, since the check that proves the packaging fix has to run somewhere other than one developer's pre-commit hook. CodeQL was already running through GitHub's default setup, but nothing was building, linting or testing the code.
Fixes #20, fixes #25.
Prerequisite knowledge
import * as N from 'notations'as a supported form. That form did not work, so this PR is mostly about making the page true.importand a non-issue underrequire.designs/ordocs/covers how the package is built and published, which is the page this change would have been checked against if it existed.Reviewer's guide
Two different postal systems deliver to this package. CJS is the one where you can write "the Smiths, Elm Street" on the envelope and the postman works out the rest, so
require("./entity")findsentity.jsby trying extensions until something sticks. ESM is the one that wants the full address including the house number, and./entitygets returned to sender. TypeScript copies whatever the source wrote straight into the output, so both builds came out addressed the loose way, and only the half delivered by the strict service failed. The other half of it is that nothing on the parcel said which service to use. Node decides that from the nearest package.json, ours declares notype, and solib/esmwas being handed to the CJS service and only worked because Node 22 opens the parcel and guesses from the contents. Older Node does not guess, and fails for a different reason instead. Both are fixed at the point the parcel is packed rather than at the point the address is written, which is why nothing insrc/moves. The CI half of the PR is the depot check: until now every sender was trusted to inspect their own envelopes before posting, which is how a whole release went out with the wrong addresses on it.Read in this order:
lib/esmand stamps both halves with their delivery service. The part worth checking isresolved(), which refuses to invent an address and fails the build instead.buildbasegained the finalize step, andcheck:esmsits in the pre-commit chain right afterbuild.Diff groups
scripts/finalize-esm.mjs,package.jsonscripts/check-esm.mjs.github/workflows/ci.ymlHow it works
Resolving against the emitted tree rather than appending
.jsblindly is what makes the seven directory imports (./,./carnatic,../../carnatic) come out as./index.jsand./carnatic/index.js. It is also what would catch a futureimport "./theme.css", which would resolve to no emitted file and stop the build rather than being quietly corrupted into./theme.css.js.check-esmthen reads the marker, scans for any relative specifier still missing an extension, and does a realawait import()plus aload()round trip.Decision log
lib/esmrather than writing.jsinto the 166 relative imports across the 49 source files. The source rewrite is the canonical TypeScript fix and it also corrects the.d.tsspecifiers at the source, but it needs amoduleNameMapperentry in jest and aresolve.extensionAliasindocs/webpack.config.js, and it would have put a 170-line mechanical diff in front of a packaging change. Worth revisiting if the project ever moves tomoduleResolution: nodenext, which forces it anyway.typemarkers to both halves rather than onlylib/esm. Pinninglib/cjscosts nothing and means neither half depends on the rootpackage.jsonstaying silent abouttype.npm ci.pnpm-lock.yamlis the tracked lockfile andpackage-lock.jsonis gitignored, sonpm cihas nothing to install from on a fresh clone.--ignore-scripts. The rootpreparescript isnpm run build, so without it CI builds twice and a broken build reports as a failed install.lintrather thanlintfixin CI, since the fixing variant rewrites files and would pass on code that is not actually clean in the tree.check:esmin the pre-commit chain as well as in CI. It needs a builtlib/, whichnpm testdoes not produce, and the local hook is still the faster feedback loop.Risk / blast radius
lib/is gitignored and rebuilt, so nothing in the repo's own working tree changes shape.importcondition go from broken to working. Consumers onrequireare unaffected, and both were checked against a real installed-package layout.lib/esm/package.jsonnow says"type": "module", which changes how webpack, rollup and vite classify those files. They already are ESM, so this should be a no-op, and the repo's own UMD build (which bundleslib/esm/index.jsthrough babel-loader) still produces the samedist/notations.umd.min.js. Worth a second pair of eyes anyway, since this is the one line with reach beyond Node.fail-fast: falsestays on so a future failure on one leg still reports the others.pnpm install --frozen-lockfilewill fail ifpnpm-lock.yamldrifts frompackage.json. It is in sync today, checked from a clean clone, and dependabot updates both together. Worth knowing that this is now a hard failure rather than a silent resolve.exportsmap. Those entries want the same treatment, and the finalizer covers any new file automatically since it walks the emitted tree.Before / after
A consumer package with
notationsinstalled,"type": "module", on Node 22.23.2.Before:
After:
The same import with module-syntax sniffing turned off, which is how Node behaves before 22.12. Before the fix it fails for a different reason than it does with sniffing on, which is the argument for the matrix:
What the emitted entry looks like:
flowchart LR subgraph after ["after"] A2["tsc -p tsconfig.json"] --> B2["lib/esm"] A3["tsc -p tsconfig-cjs.json"] --> C2["lib/cjs"] B2 --> F["finalize-esm.mjs"] C2 --> F F --> G["lib/esm with .js specifiers<br/>+ type markers on both halves"] G --> H["check-esm.mjs<br/>real Node import"] end subgraph before ["before"] A0["tsc -p tsconfig.json"] --> B0["lib/esm<br/>bare specifiers, no type marker"] A1["tsc -p tsconfig-cjs.json"] --> C0["lib/cjs"] endWhere the checks run, before and after:
CodeQL runs on PRs either way, through default setup rather than a workflow file, and is not affected by this change.
The workflow sequence was run end to end from a clean clone with a frozen lockfile:
And on this PR, which is the workflow running against itself:
Out of scope
moduleResolution: nodenext.codecovis already a devDependency but nothing is configured, sojest --coverageoutput goes nowhere.