[vitest-pool-workers] Stop externalizing devDependencies from the published bundle#13933
Conversation
🦋 Changeset detectedLatest commit: ab1852c The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
workers-devprod
requested review from
a team and
emily-shen
and removed request for
a team
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
|
✅ All changesets look good |
|
Confirmed — Now let me think through the overall changes for any logic issues:
Let me check one edge case in the comment-stripping logic — the regex Let me also verify one more subtle thing — the Let me check one more thing — the changeset bot mentions I've completed my review. The changes are well-structured, thoroughly tested, and address a real bug. The approach of deriving bundler externals from LGTM |
create-cloudflare
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
… published bundle
The bundler's external list for vitest-pool-workers was hand-maintained
and out of sync with package.json. undici and semver were both listed
as external despite being only devDependencies, leaving dist/pool/index.mjs
with a top-level import { fetch } from "undici" that only resolved at
runtime because pnpm happened to hoist undici from other packages'
devDependencies during local development.
The bundler now derives its external list from dependencies +
peerDependencies in package.json, so a devDependency can never silently
end up externalized.
Combined with the new "sideEffects": false declaration in
@cloudflare/workers-utils, the unused cloudflared / tunnel exports
(and their transitive undici import) are tree-shaken out of the pool
entirely. dist/pool/index.mjs no longer references undici at all and
shrinks from ~489 KB to ~125 KB.
@cloudflare/workers-utils also moves undici from devDependencies to
dependencies (it was always a runtime dep — the manifest just didn't
say so) and keeps it external so consumers don't end up with two
undici instances in their bundle, which would break instanceof checks
and setGlobalDispatcher / proxy configuration.
The validator (tools/deployments/validate-package-dependencies.ts) is
extended to scan each public package's runtime entry points
(main/module/exports/bin) for bare-specifier imports and flag any that
aren't declared in dependencies or peerDependencies. An
IGNORED_DIST_IMPORTS allowlist is added to scripts/deps.ts to handle
legitimate try/catch optional imports from bundled libraries
(e.g. @netlify/build-info probing for frameworks).
petebacondarwin
force-pushed
the
fix/vitest-pool-workers-undici-external
branch
from
b110bd2 to
ab1852c
Compare
|
Codeowners approval required for this PR:
Show detailed file reviewers |
workers-devprod
left a comment
workers-devprod
left a comment
There was a problem hiding this comment.
Codeowners reviews satisfied
3 participants
Fixes #13807
Summary
The bundler's
externallist for@cloudflare/vitest-pool-workerswas hand-maintained and out of sync withpackage.json.undiciandsemverwere both listed as external despite being onlydevDependencies, leavingdist/pool/index.mjswith a top-levelimport { fetch } from "undici"that only resolved at runtime because pnpm happened to hoistundicifrom other packages' devDependencies during local development. This PR fixes the bug at its source and adds a CI check to prevent it recurring.Changes
@cloudflare/vitest-pool-workerstsdown.config.tsnow derives itsexternallist fromdependencies+peerDependenciesinpackage.json. A devDependency can never silently end up externalized.@cloudflare/workers-utils"sideEffects": falseso consumers can tree-shake unused exports. With this in place,vitest-pool-workersdrops the unusedcloudflared/tunnelexports (and their transitiveundiciimport) entirely.undicifromdevDependenciestodependencies— it was always a runtime dep, the manifest just didn't say so. Keeping it external (rather than bundling) avoids two-undici-instance issues for consumers like wrangler —instanceof Request/Response/Headerschecks andsetGlobalDispatcher/ proxy configuration depend on there being a single shared undici instance.vitestas an optionalpeerDependencybecause./test-helpersusesvitest'svi/beforeEach/afterEachat runtime."msw"entry from the tsup external list — never used.Validator (
tools/deployments/validate-package-dependencies.ts)main/module/exports/bin) for bare-specifier imports and flag any that aren't declared independenciesorpeerDependencies.IGNORED_DIST_IMPORTSallowlist convention inscripts/deps.tsfor legitimate try/catch'd optional imports inside bundled libraries (e.g.@netlify/build-infoprobing for installed frameworks,recast'sbabylonfallback).Results
vitest-pool-workers/dist/pool/index.mjssizevitest-pool-workers/dist/pool/index.mjsundici referencesimport)workers-utils/dist/index.mjsundici referencesimport)wrangler-dist/cli.jsundici copiesFixes a latent runtime resolution bug and prevents regressions.