fix(build): ship type declarations in the published package#40
fix(build): ship type declarations in the published package#40dipdapdop wants to merge 4 commits into
Conversation
The published package contained no type declaration files, even though package.json advertises "types": "dist/main.d.ts" (issue kinde-oss#39). TypeScript consumers got "Cannot find module '@kinde/webhooks' or its corresponding type declarations". Root cause: vite-plugin-dts computes each declaration's output path relative to Vite's root, which was set to "lib". With outDir "../dist" that placed the .d.ts files under lib/dist, outside the configured outDir, so the plugin's strictOutput (on by default) silently skipped writing them. The package therefore shipped only the JS bundles. The "tsc &&" build step also emits nothing because tsconfig sets noEmit. Fix: remove the vestigial root "lib" and the unused "src" alias (no index.html, public dir, or "src" imports depend on them) and set the build outDir to "dist". Declarations then resolve into the repo-root dist that files ["dist"] publishes, and lib/main.ts already re-exports all the event types so consumers import from the package root. Verification: - npm pack now includes dist/main.d.ts, dist/lib/main.d.ts and dist/lib/types.d.ts. - A consumer importing the event types from "@kinde/webhooks" (package root, not a dist deep path) type-checks cleanly. - The JS bundles are byte for byte identical to the previous config (matching SHA-256 for webhooks.js, webhooks.cjs and both jsrsasign chunks), so runtime output is unchanged. - The existing test suite passes (11/11) and prettier --check is clean. Closes kinde-oss#39
Adds a suite that exercises the BUILT dist/ rather than the lib/ source, to guard the packaging regression fixed in the previous commit (kinde-oss#39). The existing suite imports from ./main, so it could not catch declarations being absent from the published tarball. The new suite asserts, against the real shipped files: - the JS entry points named by package.json main/module exist, the ESM bundle imports, and the runtime surface works (decodeWebhook is callable, WebhookEventType resolves, decodeWebhook("") returns null); - the declaration file named by package.json types exists, and a consumer importing the event types from the package root type-checks (a spawned tsc run that reproduces the exact kinde-oss#39 failure mode). Wiring: - test/dist-smoke.test.ts holds the suite (outside lib/, so the tsc build step does not compile it). - vitest.config.ts excludes test/ from the default `test` script, so the source suite still runs without a build. - vitest.dist.config.ts scopes the dist suite, run via the new `test:dist` script which builds first. Verified the suite fails on the pre-fix config (missing dist/main.d.ts and the consumer tsc errors with TS2307) and passes on the fixed config.
|
Warning Review limit reached
More reviews will be available in 48 minutes and 5 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses rolling per-developer review limits. Reviews become available again as older review attempts age out of the rolling limit window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThe Vite build config is corrected to output into ChangesDist declaration fix and smoke test
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 review |
✅ Action performedReview finished.
|
| // The default `test` script runs the source suite (lib/). The dist smoke | ||
| // suite (test/dist-smoke.test.ts) requires a prior build and is run | ||
| // separately via the `test:dist` script, so it is excluded here. | ||
| exclude: ["**/node_modules/**", "**/dist/**", "test/**"], |
There was a problem hiding this comment.
Excluding test/** from the default suite is reasonable, but the current workflow still only runs pnpm build and pnpm test:coverage. That means this new dist smoke suite never runs in CI, so the declaration-packaging regression this PR is trying to guard can slip back in without a failing PR build. Please wire pnpm test:dist into the PR workflow or fold this check into an automated path.
There was a problem hiding this comment.
@dtoxvanilla1991 Yeah good point, ta. Hopefully fixed 0537a9e.
I've added a step to build-test-ci.yml that runs the dist smoke straight after the build:
- run: pnpm build
- name: Dist smoke (declaration-packaging regression guard)
run: pnpm exec vitest run --config vitest.dist.config.ts
- run: pnpm test:coverageI run the dist config directly rather than the test:dist script so it reuses the build from the step above instead of rebuilding. It sits before test:coverage so a packaging regression will fast-fail the build. The default suite already excludes test/** and dist/**, so the two suites stay separate and the smoke runs only once.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
The dist smoke suite (test/dist-smoke.test.ts, via vitest.dist.config.ts) asserts the published dist/ ships its .d.ts and that a consumer can import the declared types. As reviewed on the PR, CI only runs build and test:coverage, so that suite never executed in CI and the declaration-packaging regression this PR fixes could silently return without failing a PR build. Add a step after the existing build that runs the dist config directly (pnpm exec vitest run --config vitest.dist.config.ts) rather than the test:dist script, so it reuses the build from the step above instead of rebuilding. No untrusted workflow input is referenced.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@test/dist-smoke.test.ts`:
- Around line 109-112: The current implementation uses execFileSync to invoke
the tsc binary directly from node_modules/.bin/tsc, which fails on Windows due
to npm creating .cmd wrapper shims. Instead of executing the tsc binary
directly, invoke TypeScript via Node by passing "node" as the command to
execFileSync and the path to the TypeScript CLI as the first argument in the
arguments array. This ensures cross-platform compatibility by using Node to
execute the TypeScript package rather than relying on shell-specific binary
wrappers.
- Around line 51-53: The dynamic import() calls at lines 52 and 59 use raw
filesystem paths which cause Windows compatibility failures because the drive
letter is interpreted as a URL protocol scheme. Convert both filesystem paths
created by join(repoRoot, pkg.module) to file:// URLs using Node.js's
pathToFileURL utility from the url module before passing them to the import()
function. This ensures the paths are properly formatted for cross-platform
compatibility.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 595a424d-0c24-4e00-95f1-f9413d3e91ca
⛔ Files ignored due to path filters (2)
.github/workflows/build-test-ci.ymlis excluded by!**/*.ymlpackage.jsonis excluded by!**/*.json
📒 Files selected for processing (4)
test/dist-smoke.test.tsvite.config.tsvitest.config.tsvitest.dist.config.ts
Two cross-platform fixes from review: 1. Dynamic import() was passed a bare filesystem path. On Windows a drive-letter path is parsed as a URL scheme and throws ERR_UNSUPPORTED_ESM_URL_SCHEME. Wrap the path in pathToFileURL(...).href via a small importPath() helper at both call sites. 2. The tsc consumer-typecheck spawned node_modules/.bin/tsc. On Windows that shim is a .cmd wrapper that execFileSync (no shell) cannot resolve. Invoke node against the typescript package's JS bin entry instead, which is OS-neutral. Verified locally: build + the dist config run green (5/5). No untrusted input.
2 participants
Explain your changes
Fixes #39.
Problem
@kinde/webhooksships no.d.tsfiles despitepackage.jsonadvertising"types": "dist/main.d.ts", so TypeScript consumers getCannot find module '@kinde/webhooks' or its corresponding type declarations.Root cause
vite-plugin-dtscomputes each declaration's output path relative to Vite'sroot, which was"lib".With
outDir: "../dist"the.d.tsfiles were written underlib/dist/(outside the configuredoutDir), so the plugin'sstrictOutput(on by default) silently skipped writing them.tsc &&build step also emits nothing becausetsconfigsetsnoEmit.Fix
Remove the left-over
root: "lib"and the unusedsrcalias (nothing depends on them:no
index.html, nopublic/, no"src"imports) and set the buildoutDirto"dist".Declarations then resolve into the repo-root
dist/thatfiles: ["dist"]publishes.lib/main.tsalready re-exports the event types, so consumers import from the package root.Verification
npm packnow includesdist/main.d.ts,dist/lib/main.d.tsanddist/lib/types.d.ts.@kinde/webhooks(package root, not adist/deep path) type-checks cleanly.SHA-256 for
webhooks.js,webhooks.cjsand bothjsrsasignchunks), soruntime output is unchanged.
prettier --checkis clean.pnpm test:dist) that imports the built bundle andtype-checks a root import, guarding this regression. Verified it fails on the
pre-fix config (
TS2307) and passes on the fix.Checklist
🛟 If you need help, consider asking for advice over in the Kinde community.