[ci]: gate the public API surface and the published package shape - #1471
Open
sf-tyler-jeong wants to merge 10 commits into
Open
sf-tyler-jeong wants to merge 10 commits into
sf-tyler-jeong wants to merge 10 commits into
Conversation
The published package exposes 202 subpath entries, and what each one declares is only visible in the generated .d.ts tree — which the build throws away. So a change like dropping a context action or turning Promise<void> into void leaves no trace in the diff: tsc stays green because the library is internally consistent, and the break surfaces in a customer's build instead of ours. `yarn api:snapshot` walks the entry points in rollup.module-exports.mjs, follows re-exports and inline import types through dist/types, and writes the reachable declarations to api/surface.d.ts. Internal modules that no entry point reaches are left out — 294 of 546 files survive the filter. The file is generated, never edited. Regenerating it twice over one build gives a byte-identical result.
Regenerates the snapshot after the build and compares it with the committed copy. A mismatch means the public surface moved without anyone recording it, so the step prints the diff and fails; an intended change passes once api/surface.d.ts is regenerated and committed alongside the code. The job name is unchanged, so the existing required check covers this without touching the ruleset, and `yarn build` already runs here — the step adds no build time. It also asserts the snapshot is tracked. Left untracked, `git diff` reports no change and the gate would pass silently forever.
The snapshot is only as good as the walk that builds it. If following `export *`, a directory index, or an inline `import()` type ever regresses, the file shrinks, the diff looks empty, and the gate reports a clean surface while saying nothing — the failure mode with no symptom. Splits the walk out of the CLI so it can be driven against a fixture tree and covers it with fourteen cases: entry mapping, specifier resolution, star and named re-exports, inline import types, cycles, several entries merging, an entry whose declaration was never emitted, and the snapshot's format and determinism. Verified by breaking the walk — dropping inline import tracking fails one case, dropping index resolution fails two. vitest looked only under src/, so the include now reaches scripts/ as well. The generated snapshot is byte-identical to the one already committed. Two smaller repairs: the workflow now says why a missing api/surface.d.ts matters instead of leaving a bare pathspec error, and the script warns when src/ is newer than dist/types, since a stale tree snapshots cleanly and the mismatch would only appear in CI.
The script decided whether to run by comparing process.argv[1] with import.meta.url, so that it could also be imported by the spec. Node resolves symlinks for the module URL but leaves argv[1] as given, so reaching the repo through a symlinked path made the two differ and main() never ran. The script then exited 0 with no output, the snapshot was never regenerated, `git diff` came back empty, and the gate reported an unchanged surface. No error, no warning. On macOS /tmp is a symlink to /private/tmp, which is how the throwaway worktree used while building this reached the repo. Splitting the CLI into its own file removes the decision rather than correcting it — importing the module now does nothing on its own, so there is no condition left to get wrong. Verified through both a symlinked and a direct path; the generated file is unchanged.
…pshot Several public paths map to the same source file, so the snapshot was byte-identical after one of them was deleted: the declaration stayed reachable through its sibling. Dropping 'Channel/components/MessageInput' left the gate green while '@sendbird/uikit-react/Channel/components/MessageInput' started failing with ERR_PACKAGE_PATH_NOT_EXPORTED for anyone importing it. The walk only consumed Object.values(moduleExports), discarding the path names entirely. Emit them as a header ahead of the declarations, sorted by public path so the block does not move with declaration order. Covered by five cases that go through renderSnapshot rather than renderEntryPoints: asserting the pure function leaves them all passing when the header is unwired from the written file.
`lame.all` is the only entry whose source is plain JavaScript, so the `.replace(/\.tsx?$/, '.d.ts')` that derives the declaration path silently left it alone. The published manifest named types/_externals/lamejs/lame.all.js under both `exports` and `typesVersions`, and tsc --emitDeclarationOnly never writes that file: publint reports it as an error and the directory is absent from the tarball. Omit the `types` condition when the source emits no declaration. Against the manifest published as 3.19.0 the only differences are the dropped key and its typesVersions entry; the other 203 export entries are unchanged. attw moves the subpath from NoResolution to UntypedResolution, so consumers go from finding nothing to finding the JavaScript without types. Extract the map into its own module so the shape can be asserted without running the build.
movePackageJSON() was called without await, so its write raced the synchronous work that followed it. execSync blocks the loop until it returns, which means a failure in buildTypeDefinitions() ends the process before the continuation ever runs and dist/package.json is left missing. Observed while running the script with tsc off PATH: exit 127, no manifest. Sequence the steps and report a failure explicitly rather than as an unhandled rejection.
The surface snapshot proves the declarations did not change. It says nothing about whether a consumer can reach them: it walks dist/types from the entry sources, so an exports target that resolves nowhere is absent from the snapshot instead of visible in it. Add publint and attw over dist. Known-failing categories are excluded rather than fixed here, so the gate reports regressions only: - false-esm: `dist/package.json` declares "type": "module" while `dist/cjs` declares commonjs, so a node16/nodenext consumer emitting CommonJS gets TS1479 on every entry. bundler, legacy node and ESM consumers all typecheck. Worth its own change: correcting it means emitting a second set of declarations for the require condition across all 204 entries. - internal-resolution-error: relative specifiers inside the declaration files. A consumer with skipLibCheck disabled reports none of them, so this is not reachable from outside the package. Both entries excluded by path are untyped by nature: the stylesheet and lame.all.
c661f806 ordered the steps so dist/package.json is written before anything that can throw, but nothing held that ordering: the suite covers the export map alone and stays green when the await is dropped and tsc fails. Run the real script in a sandbox rather than against dist/, which the surface snapshot and package:check both read. A temporary tree gets the three scripts, a single-entry module map and empty dist directories, and a stub tsc ahead of PATH makes the declaration step fail the same way on any machine. Two assertions, each covering a different way the fix can rot: dropping the await loses the manifest, and swallowing the rejection loses the exit code. Neither catches the other, since an unhandled rejection also exits non-zero.
Nothing read the TypeScript this branch added under scripts/. eslint is scoped to 'src/**/*.ts*', tsconfig.json includes only src and vitest-setup.ts, and vitest strips types without checking them: a `const x: number` assigned a string passed typecheck, lint and test alike. A separate project rather than widening tsconfig.json, because the two options that touch the shared config both fail. Adding "scripts" to include alone leaves the imports from .js and .mjs typed as any, since noImplicitAny is off, so the gate reports nothing. Adding allowJs there reaches the build as well — tsconfig.build.json overrides include but inherits compilerOptions — and the declaration emit picks up five mock files under src that its exclude list does not cover, putting them in the published package. Scoped this way the emit is untouched at 546 declarations, and a type error planted in each of the three specs now fails yarn typecheck. The spec change is a cast: package_exports.js assigns its export entries through a computed key, which leaves Object.values inferring string[].
|
| Commit | Scanned at | New | Resolved | Net |
|---|---|---|---|---|
001d7b3 < |
2026-09-18 03:51 UTC | 0 | 0 | 0 |
Last scanned: 001d7b3 · 2026-09-18 03:51 UTC
✅ Deploy Preview for sendbird-uikit-react ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
| Commit | Scanned at | New | Resolved | Net |
|---|---|---|---|---|
001d7b3 < |
2026-09-18 03:52 UTC | 0 | 0 | 0 |
Last scanned: 001d7b3 · 2026-09-18 03:52 UTC
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
This branch has not been deployed
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
1 participant
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
Public API surface changes are caught by a person reading a diff. Props, exports, types and the shape a context or hook returns are all contracts a customer builds against, and once one of them slips out it cannot be taken back.
This checks a machine-written snapshot of that surface into the repository and has CI regenerate and compare it on every build. A surface that moves fails the build. An intended change is committed together with the snapshot, so the diff says exactly what moved.
The same idea is applied to the shape of the published package, with
publintand@arethetypeswrong/cli. Turning that on surfaced two real defects, fixed here.Changes
The surface snapshot —
api/surface.d.tsStarting from the entry points in
rollup.module-exports.mjs, the generator follows relative imports throughdist/typesand collects every declaration reachable from them. Package specifiers are not followed: those types belong to their own packages. The result is sorted by path, so traversal order cannot move the file.The snapshot opens with a map of entry point name to source path. Without it there is a hole. Removing an entry point does not necessarily remove any declaration, because another entry point can already reach it —
Channel/components/MessageInputandChannel/components/MessageInputWrapperpoint at the same source today. The declarations section would be byte-identical and the gate would pass on a removed public path.CI regenerates after
yarn buildand compares withgit diff --quiet. It first refuses to run ifapi/surface.d.tsis untracked, since a deleted file makes the comparison pass silently.The published package shape —
yarn package:checkpublintreads the manifest andattwresolves every entry point the way a consumer would. Both were run by hand first, and what they turned up is fixed below.Two rule classes the package already triggered before this change —
false-esmandinternal-resolution-error— are excluded, so the gate reports regressions instead of requiring the existing baseline to be cleared first. The two entry points excluded by path are untyped by nature: the stylesheet andlame.all.Two defects it found
The manifest pointed at a declaration file the package never shipped.
lame.allis the only entry point whose source is plain JavaScript, so the.replace(/\.tsx?$/, '.d.ts')that derives the declaration path matched nothing and left the.jsextension in place. BothexportsandtypesVersionsnamedtypes/_externals/lamejs/lame.all.js, whichtsc --emitDeclarationOnlynever writes — the directory is not in the tarball.publintreports it as an error. Thetypescondition is now omitted when the source emits no declaration. Against the manifest published as 3.19.0, the only differences are that key and itstypesVersionsentry; the other 203 export entries are unchanged.A failed build left the manifest unwritten.
post_build.jscalledmovePackageJSON()without awaiting it, so its write raced the synchronous work behind it.execSyncholds the loop, so a failure in the declaration build ended the process before the continuation ran anddist/package.jsonwas left missing. The steps are sequenced and a failure is now reported explicitly instead of as an unhandled rejection.Type checking for
scripts/Nothing read the TypeScript this branch adds under
scripts/: eslint is scoped tosrc/**/*.ts*,tsconfig.jsonincludes onlysrc, and vitest strips types without checking them. A separate project rather than widening the shared config: settingallowJsthere would reach the build too, sincetsconfig.build.jsonoverridesincludebut inheritscompilerOptions. Measured, that alternative grows the declaration emit from 546 to 551 and puts five mock files into the package, so it was rejected. Scoped this way the emit is untouched.Backward compatibility
No public API changes. Not a line of
src/is touched, and props, exports, types, context return shapes, CSS class names and StringSet keys are all unchanged.The one manifest difference is the
typespointer removed fromlame.all, and it changes nothing for a consumer. Measured against a package with the old shape and one with the new, TypeScript reports the sameTS7016and resolves to the same JavaScript file underbundler,node16andnode10alike — it falls through atypescondition that resolves to nothing. A control with a real.d.tscompiles clean, so the comparison does distinguish the cases. What changes is that the manifest no longer names a file that is not in the package, which is whatpublintandattwread.Testing
yarn lint,yarn typecheck,yarn build,yarn test,yarn api:snapshotandyarn package:checkall passui/Togglefromrollup.module-exports.mjsand regenerating drops 55 lines from the snapshot, including its entry point line, andgit diff --quietfailsgit diff --exit-code:renderSnapshot→ 5 failures (ordering, removal, rename, repoint, header placement)awaitout ofpost_build.js→ 1 failure (manifest present when the declaration build fails)nullreturn out ofdeclarationPath→ 4 failures (types omitted, typesVersions omitted, target inside the shipped tree).d.tsfiles into a temporary directory and cover star re-exports, directory indexes, inlineimport()types in a signature, cycles, and several entry points sharing a closurepackage:checkexits zeroChangelogs