fix: check that subpaths IMPORT, not just that files exist (#70) - #71
Merged
Merged
Conversation
check:exports (added last release) asserts every `exports` target is present in
dist/. That caught a real stale list, but presence is weaker than importability:
a file can exist and still be unloadable. ./terminal shipped in BOTH 0.6.0 and
0.7.0 present-but-unimportable under plain Node, and the guard reported "all 19
targets present" the whole time.
Found by installing the published 0.7.0 tarball and importing all nine subpaths
— not by anything in the repo, which is the point of adding it here.
New check:imports imports each code subpath for real. Bundler-only subpaths are
DECLARED WITH A REASON rather than skipped, and it fails in both directions:
- an undeclared subpath that stops importing (verified: added a CSS import to
dist/quotas.js → fails)
- a declared one that starts working (verified: listed ./quotas → fails)
The second direction matters as much as the first. A restriction we still
document to consumers after it has gone away is its own bug, and nothing else
would ever notice.
./terminal is declared bundler-only. I first tried to FIX it by dropping the
`import "@xterm/xterm/css/xterm.css"` side effect and pushing it to the two
bundled consumers, mirroring how ./ui/style.css already works. That was wrong
and is reverted: the CSS is the smaller of two reasons, and the other is
upstream. @xterm/xterm publishes no "exports" field, so Node resolves its CJS
`main` and ignores the ESM `module` build — and the two disagree on shape
(xterm.mjs has no default export; Node's CJS view exposes Terminal only under
`default`), so no single import form satisfies both. Dropping the CSS import
would break styling for every current bundled consumer while leaving the subpath
bundler-only regardless: all cost, no benefit.
Also from the same pass:
- LIB_VERSION is exported from the package root. It is stamped into
spawn:version on every launch but was readable only from the tag on an
already-launched instance — a consumer could not report its own version until
after it had launched something.
- README gains an entry-point table for all ten subpaths, naming ./ssm as the
xterm-free alternative to ./terminal. It also still advertised the pre-library
"spawn-ts" import name, which has been wrong since 0.6.0.
- repository.url is now git+https://…, the form npm was silently normalising it
to on every publish.
591 tests pass; check:exports and check:imports both wired into release:check
and publish.yml.
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
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.
Closes #70.
check:exports(added last release) asserts everyexportstarget is present indist/. That caught a real stale list — but presence is weaker than importability, and the gap was not theoretical:./terminalshipped in both 0.6.0 and 0.7.0 present-but-unimportable under plain Node, while the guard reported "all 19 targets present" the whole time.Found by installing the published 0.7.0 tarball and importing all nine subpaths — not by anything in the repo. That's the reason to add it here.
npm run check:importsImports each code subpath for real, from the built
dist/. Bundler-only subpaths are declared with a reason rather than skipped, and it fails in both directions:dist/quotas.js→ fails./quotasas bundler-only → failsThe second direction matters as much as the first: a restriction we still document to consumers after it has gone away is its own bug, and nothing else would ever notice. The summary line also states the bundler-only count separately rather than folding it into a total — "all 9 import" would be false while one provably does not.
What I got wrong first
I tried to fix
./terminalby dropping itsimport "@xterm/xterm/css/xterm.css"side effect and pushing the CSS to the two bundled consumers, mirroring how./ui/style.cssalready works. Reverted — the CSS is the smaller of two reasons, and the other is upstream:@xterm/xtermpublishes noexportsfield, so Node resolves its CJSmainand ignores the ESMmodulebuild.xterm.mjshas no default export, while Node's CJS view exposesTerminalonly underdefault. No single import form satisfies both.So dropping the CSS import would have broken styling for every current (bundled) consumer while leaving the subpath bundler-only anyway — all cost, no benefit. It's now declared, with that reasoning recorded where the next person will look.
Also from the same pass
LIB_VERSIONis exported from the package root. It's stamped intospawn:versionon every launch, but was readable only from the tag on an already-launched instance — a consumer couldn't report which version it was running until after it had launched something../ssmas the xterm-free alternative to./terminal. The README also still advertised the pre-library"spawn-ts"import name, wrong since 0.6.0.repository.url→git+https://…, the form npm was silently normalising on every publish, so published metadata no longer differs from the repo's.Verification
591 tests pass, typecheck clean. Both guards wired into
release:checkandpublish.yml:No published artifact changes here beyond the metadata and the new root export, so this rides the next release rather than needing one.