Repository navigation
ci: check package exports and declarations with publint and attw - #39
Merged
Merged
Conversation
Nothing in CI looked at the package from a consumer's point of view.
`npm run type-check` covers src/ only, and `npm run build` succeeding
says nothing about whether the emitted declarations resolve for someone
importing the package. Reviewing the tsup 9 bump meant packing the
tarball and type-checking a throwaway consumer against it by hand to
rule out a dts regression on the react/server subpaths.
attw is what closes that gap: a declaration file that imports a chunk
the build never emitted passes publint and fails attw with an internal
resolution error on exactly those subpaths. publint covers the
complementary cases — missing files, unpublished files, crossed
ESM/CJS types conditions.
attw runs with --profile node16. The node10 profile fails today for the
./react and ./server subpaths because legacy resolution cannot see
exports subpaths. typesVersions would turn those cells green, but only
for types: `require('@jackmisner/utm-toolkit/react')` still has nothing
to resolve on disk for that cohort, so it would publish types that lie.
Declaring node10 unsupported is the honest option.
The job runs once rather than per Node version; the build output is
byte-identical across 22/24/26.
… its deps Three review findings. The package-check job inherited `contents: write` and `pull-requests: write` from ci.yml and needs neither; it only reads the repo, installs, builds and packs. A called workflow can only reduce the caller's scope, so declaring `contents: read` here is enough. publish.yml built twice: the standalone step, then `check:package`'s own `npm run build`. tsup runs with `clean: true`, so the first build's output was deleted before anything looked at it. publint and @arethetypeswrong/cli get their own Dependabot group. Both are pre-1.0, where a breaking change ships as a minor, and either can start failing `check:package` against unchanged package code. In the grouped npm-minor-patch PR that would block every unrelated bump behind it.
…olicy A Node 20+ CommonJS TypeScript consumer with the default moduleResolution gets TS2307 on @jackmisner/utm-toolkit/server and /react while the same install resolves fine at runtime, because TypeScript's legacy resolution cannot see exports subpaths. Nothing said so; the README's "All three ship dual ESM/CJS builds with TypeScript declarations" reads as a promise that those subpaths type-resolve anywhere. The earlier rationale for --profile node16 was that typesVersions would publish types that lie. That is wrong about who is affected: the cohort it describes, exports-blind bundlers, already fails loudly at bundle time and cannot use the subpaths either way. For the cohort that actually hits this the runtime works, so typesVersions would be accurate rather than a lie. It is still deliberately not added, but as a scope and maintenance call rather than a correctness one, and that is now written down where the other support-matrix decisions live.
… in Platform Support `moduleResolution: bundler` needs TypeScript 5.0, not 4.7: on 4.9.5 it is rejected with TS6046 while `node16` is accepted, so the single version floor covered two of the three settings and not the third. Platform Support is where a server consumer checks what is supported, and its Node.js list ended by calling /server the supported server surface with no mention of the one constraint a TypeScript consumer of it hits. The TypeScript Types section above shows that exact subpath type import with no caveat, so a reader could land there and copy an import that does not resolve under their default settings. The typesVersions clause recorded who decided rather than what to do; it now reads as the instruction a dev reaching for it needs.
"The node10 row fails" sets up a reader to expect check:package to go red. Under --profile node16 attw still prints the row, marks every cell (ignored) and leaves it out of the exit code, so the wording described the wrong consequence. Scopes the runtime claim to Node, since an exports-unaware resolver is exactly what a consumer stuck on node10 resolution may be using, and trims the Dependabot group comment to the part the YAML cannot express: that the absent update-types filter is deliberate.
jackmisner
added
documentation
Merged
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.
Why
Nothing in CI looked at this package the way a consumer does.
npm run type-checkistsc --noEmitoversrc/only, andnpm run buildsucceeding says nothing about whether the emitted declarations resolve for someone importing the package.That gap surfaced while reviewing the tsup 9 bump (#38): confirming it had not broken the
/reactand/serverdeclarations meant packing the tarball by hand and type-checking a throwaway consumer against it. A real regression there would have shipped on a green CI.What changed
publintand@arethetypeswrong/clias devDependencies, behind one script:check:package=npm run build && publint --strict && attw --pack . --profile node16.github/workflows/package-check.yml, a reusable workflow called fromci.ymlas apackage-checkjob (needs: format), withpermissions: contents: readCheck package exports and declarationsstep inpublish.yml'scigate job, which is the jobpublishhangs off vianeeds: cipackage-checkDependabot group for the two toolsREADME.mdandCLAUDE.mdrecord the node10 resolution non-target and the check's policyThe check runs once rather than per Node version: the build output is byte-identical across 22/24/26, verified by diffing the bundles while reviewing #38.
How it was verified
The check has to fail on breakage, so each case below is a deliberate break of the real package, run through the real tools and reverted:
exports["./react"].import.types→ non-existent filerequire.types→ the ESM declarationrequirecondition"distremoved fromfilesdist/react/index.d.tsimports a chunk the build never emittedInternalResolutionErroron./reactThe fourth case is the one that matters: the first three all short-circuit on publint, so without it attw would have gone in unproven. It is also the regression class that motivated the PR, and publint does not catch it. The inverse holds too — attw exits 0 on a tarball containing no declarations at all, so publint is what catches that. Neither tool is redundant;
CLAUDE.mdrecords this so neither gets dropped as duplicated effort.Also run on the final tree:
type-check,lint,format:check, 635 tests across 21 files,npm run build, andcheck:packagefrom a coldnpm ci.The node10 decision
attw --pack .fails onmaintoday, before this branch: legacy resolution cannot seeexportssubpaths, so./reactand./serverreportResolution failed. attw runs with--profile node16, which drops only the node10 row from the exit code while still printing it.My first rationale for that was wrong and the branch says so. I argued
typesVersionswould publish types that lie, on the grounds that anexports-blind bundler still has nothing to resolve. Review showed that cohort already fails loudly at bundle time and cannot use the subpaths either way. The cohort that actually hits this is a Node 20+ CommonJS TypeScript project on the default resolution:require('@jackmisner/utm-toolkit/server')works at runtime whiletscgivesTS2307. For themtypesVersionswould be accurate, not a lie.It is still left out, but as a scope call rather than a correctness one, and the constraint is now documented instead of implied.
--ignore-rules no-resolutionwas rejected on evidence: it suppressesNoResolutionacross every profile and paints node10 green, hiding future regressions.Not in this PR
typesVersionsfor the two subpaths — changes the published package surface, so it gets its own PR.permissions: contents: readonci-node22/24/26.yml— they still inheritcontents: write+pull-requests: writefromci.yml. Same finding as the one fixed here, pre-existing.publishjob's own redundant build —Build packagefollowed bynpm publish, whoseprepublishOnlyrunsclean && build && test, so that build's output is deleted before it is used. The same waste this PR removed from thecijob, pre-existing in the other job.12017343has onlydeletion,non_fast_forwardandpull_request. No CI check blocks a merge tomaintoday, this one included. Being handled separately, once the real check-context names are readable off this PR's run.Review
Reviewed by three agents over
78e2750...8d17573and again over the fix commits, plus a claim audit on the final diff. Findings acted on: the token narrowing, the redundant build, the Dependabot group, the corrected node10 rationale, a wrongbundlerTypeScript floor (it needs 5.0, not 4.7 — TS 4.9.5 rejects it withTS6046), and a missing Platform Support cross-reference. The audit also caughtCLAUDE.mddescribing the node10 row as failing, which would lead a reader to expect a red check when--profile node16prints it marked(ignored)and leaves it out of the exit code.