refactor: generate types with typescript instead of webpack/tooling - #675
Conversation
`webpack/tooling` bundled every reachable type into a single hand-rolled `types.d.ts`, which meant a second type system to keep in sync with the JSDoc it was derived from. TypeScript emits the same information itself, so `tsc` now generates the declarations straight from `lib/`. The declarations live in `types/`; `types.d.ts` stays the entry point and re-exports them, so the `types` field and any direct reference to `enhanced-resolve/types.d.ts` keep resolving. `scripts/generate-types.js` writes them and, with `--check`, fails on a stale checkout by emitting into a temporary directory and comparing, so CI reports drift without leaving a dirty working tree. Per-file emit only exports what the entry point re-exports, so the four plugin classes needed `@typedef` aliases in `lib/index.js` to stay usable as types rather than only as constructors. `test/types/surface.ts`, run by `lint:types-api`, now pins the whole public surface: every previously exported name is used there in the position it has to support, so a typedef that stops being re-exported fails the build instead of silently disappearing from the package. Two shapes follow the sources more closely than the old generator did. The object form of `Plugin` no longer declares `this: Resolver` on `apply` - it is invoked as `plugin.apply(resolver)`, so `this` is the plugin, not the resolver - and the entries of `ResolveContext.stack` declare `name: string | undefined` rather than an optional `name`. Class fields the old generator dropped, such as the cache backends on `CachedInputFileSystem`, are now part of the declarations. Dropping the dependency removes 32 transitive packages from the lockfile. It also drops the `inherit-types` check, which copied JSDoc from base methods onto overrides; the sources already carry the results. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016aGHrb1YaEvaGwNELGEHjF
The declarations in `types/` are generated from the JSDoc in `lib/`, so a
checkout whose types were never regenerated ships declarations that do not
describe the code. Nothing caught that in CI.
The lint job now regenerates them and diffs, which is how
`webpack-dev-middleware` and `schema-utils` check theirs:
- name: Build types
run: npm run build:types
- name: Check types
run: git status types --porcelain # non-empty => fail
`build:types` replaces the bespoke `--check` mode that compared against a
temporary directory; git already answers that question, and the script is
smaller for it. It removes `types/` before emitting, because `tsc`
overwrites the declarations it still emits but never deletes the ones it no
longer does - and a leftover file is invisible to a `git status` check,
being tracked, unchanged, and still published after its source is gone.
`types/` is no longer in `.prettierignore`: `build:types` formats it, so
`fmt:check` covers it like any other source. `lint:special` is gone from
the `lint` chain, since the CI steps above are what check this now.
Verified: regenerating on a clean tree leaves it clean; a JSDoc change
without regeneration is reported; a deleted source file removes its
declaration rather than leaving it behind.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016aGHrb1YaEvaGwNELGEHjF
🦋 Changeset detectedLatest commit: cf0ffcb The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #675 +/- ##
=======================================
Coverage 98.44% 98.44%
=======================================
Files 52 52
Lines 10749 10753 +4
=======================================
+ Hits 10582 10586 +4
Misses 167 167
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
CodSpeed Performance Analysis is red ("Performance Regression: -13.35%", two regressed benchmarks). This is not a regression from this PR, and there is no fix to port. This branch contains no executable change. The whole diff to Everything else is The one thing in the diff that could plausibly have reached a benchmark is the lockfile: dropping CodSpeed's own summary says why the numbers moved:
This has a precedent here: #673 changed nothing but YAML and was reported at -85.11%. Acknowledging the regressions in the CodSpeed dashboard is a maintainer action, so I am leaving that to you rather than touching the benchmarks. I am re-running the check once in case it lands on a matching runner; if it comes back red on the same warning, it is measurement noise rather than anything in this diff. Generated by Claude Code |
WalkthroughThe package now generates declaration files with TypeScript from JSDoc in Priority: ⬇️ Low Merge Risk: 🟡 Moderate · up to Type generation fails on supported Node versions below 14.14, so the script or declared engine range should be corrected before merge. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 15425a7b-ab09-4102-a6a3-7f8f385cbb9c
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (68)
.changeset/generate-types-with-typescript.md.github/workflows/test.yml.prettierignoreeslint.config.mjslib/index.jspackage.jsonscripts/build-types.jstest/types/surface.tstsconfig.types.api.jsontsconfig.types.jsontypes.d.tstypes/AliasFieldPlugin.d.tstypes/AliasPlugin.d.tstypes/AliasUtils.d.tstypes/AppendPlugin.d.tstypes/CachedInputFileSystem.d.tstypes/CloneBasenamePlugin.d.tstypes/ConditionalPlugin.d.tstypes/DescriptionFilePlugin.d.tstypes/DescriptionFileUtils.d.tstypes/DirectoryExistsPlugin.d.tstypes/ExportsFieldPlugin.d.tstypes/ExtensionAliasPlugin.d.tstypes/FileExistsPlugin.d.tstypes/ImportsFieldPlugin.d.tstypes/JoinRequestPartPlugin.d.tstypes/JoinRequestPlugin.d.tstypes/LogInfoPlugin.d.tstypes/MainFieldPlugin.d.tstypes/ModulesInHierachicDirectoriesPlugin.d.tstypes/ModulesInHierarchicalDirectoriesPlugin.d.tstypes/ModulesInRootPlugin.d.tstypes/ModulesUtils.d.tstypes/NextPlugin.d.tstypes/PackageMapPlugin.d.tstypes/ParsePlugin.d.tstypes/PnpPlugin.d.tstypes/Resolver.d.tstypes/ResolverFactory.d.tstypes/RestrictionsPlugin.d.tstypes/ResultPlugin.d.tstypes/RootsPlugin.d.tstypes/SelfReferencePlugin.d.tstypes/SymlinkPlugin.d.tstypes/SyncAsyncFileSystemDecorator.d.tstypes/TryNextPlugin.d.tstypes/TsconfigPathsPlugin.d.tstypes/UnsafeCachePlugin.d.tstypes/UseFilePlugin.d.tstypes/createInnerContext.d.tstypes/forEachBail.d.tstypes/getInnerRequest.d.tstypes/getPaths.d.tstypes/index.d.tstypes/util/entrypoints.d.tstypes/util/fileURLToPath.d.tstypes/util/fs.d.tstypes/util/graceful-fs-browser.d.tstypes/util/identifier.d.tstypes/util/memoize.d.tstypes/util/module-browser.d.tstypes/util/packageMap.d.tstypes/util/path-browser.d.tstypes/util/path.d.tstypes/util/pathToFileURL.d.tstypes/util/process-browser.d.tstypes/util/strip-json-comments.d.tstypes/util/url-browser.d.ts
💤 Files with no reviewable changes (1)
- .prettierignore
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.
Three findings from the review on #675, each verified against the sources before changing anything: `scripts/build-types.js` ran the `node_modules/.bin` shims, which are `.cmd` files on Windows and so needed `shell: true`. A shell receives the command line verbatim, so a checkout under a path containing a space would not start - `C:\Users\Jane Doe\...` splits at the space. It now runs each tool's JavaScript entry point under `process.execPath`, with no shell and nothing to quote. The browser stub for `graceful-fs` declared `unavailable()` with no parameters, so the declaration this package now ships rejected the calls its own callers make: `readFile(path, callback)` did not compile for anyone importing the subpath. It takes a rest parameter now. Every alias still throws; only the declared arity changed. `PackageMapOptions.configFile` was `string | null`, but the only code that constructs `PackageMapPlugin` is `normalizePackageMap`, which throws unless a config file is given - the property is never null in practice, which is why reading it needed a cast. Narrowing it to `string` makes the declaration honest and removes the cast. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016aGHrb1YaEvaGwNELGEHjF
|
codecov/project is red on The Test run for Measured locally, on one platform and one Node.js version so the comparison is like for like:
Identical. The +4 lines and +4 hits are the four Codecov's figure for the base is 10582 hits, 9 above what the same measurement gives here — and that same 9-hit gap appears on the base and on the head alike. Those 9 lines are platform-specific code that only the Windows and macOS legs execute, so until those legs upload, the head is being compared against a base that includes them. 98.36% is exactly the ubuntu-only number. I also checked that this commit changed nothing: comparing So there is nothing to fix and no fix to port. This should clear itself once the remaining legs upload; I am watching it and will follow up if it does not. Generated by Claude Code |
`scripts/build-types.js` existed to run `tsc` and `prettier` and to clear
`types/` first. npm already puts both binaries on PATH and handles the
Windows shims, so the script bought nothing that a script field cannot do,
and the clearing is one `node -e` away:
"build:types": "node -e \"require('fs').rmSync('types', { recursive: true, force: true })\"
&& tsc -p tsconfig.types.json
&& prettier --log-level warn --write \"types/**/*.d.ts\""
That also settles the Windows quoting the review raised, further up the
stack than the previous fix: npm invokes the tools, so there is no
`spawnSync` of ours to get wrong.
`test/types/surface.ts` and `lint:types-api` are gone too. They guarded
against an export silently disappearing, but the `@typedef` re-exports in
`lib/index.js` are already that guard: they are what TypeScript emits the
export list from, `types/` is generated and committed, and CI regenerates
and diffs it. Dropping a typedef therefore changes a committed file and
fails the `Check types` step - a second copy of the API in a test file was
restating what the sources already say.
Verified rather than assumed: `lib/index.js` declares 19 typedefs,
`types/index.d.ts` exports exactly those 19, and they are exactly the 19
names `webpack/tooling` exported at 332d999 - nothing lost, nothing added.
Deleting the `ResolveRequest` typedef drops it from the declarations
entirely and shows up as a modified `types/index.d.ts`. A consumer
installed from `npm pack` imports all 19, including the four plugin
classes in type position, and compiles.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016aGHrb1YaEvaGwNELGEHjF
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: f2ab9464-a713-4fe0-8f80-548de55d6013
📒 Files selected for processing (2)
eslint.config.mjspackage.json
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.
| "fix": "npm run fix:code && npm run build:types", | ||
| "fix:code": "npm run lint:code -- --fix", | ||
| "fix:special": "node node_modules/tooling/inherit-types --write && node node_modules/tooling/generate-types --write", | ||
| "build:types": "node -e \"require('fs').rmSync('types', { recursive: true, force: true })\" && tsc -p tsconfig.types.json && prettier --log-level warn --write \"types/**/*.d.ts\"", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep build:types compatible with the declared Node engine.
package.json declares engines.node as >=10.13.0, but fs.rmSync was added in Node.js 14.14.0. On supported Node versions below 14.14.0, npm run build:types fails before tsc runs. Either raise the engine floor to >=14.14.0 or replace this call with an API supported by the declared engine range. (nodejs.org)
There was a problem hiding this comment.
I checked this one rather than taking it at face value, and I don't think it holds — the described failure is unreachable.
engines.node: ">=10.13.0" states what the published library supports at runtime, for consumers. It has never described what building it needs. The dev toolchain's own floors:
| package | version | engines.node |
|---|---|---|
| typescript | 6.0.3 | >=14.17 |
| prettier | 3.9.8 | >=14 |
| eslint | 9.39.5 | ^18.18.0 || ^20.9.0 || >=21.1.0 |
| husky | 9.1.7 | >=18 |
| cspell | 10.3.3 | >=22.18.0 |
| lint-staged | 17.5.1 | >=22.22.1 |
The relevant one is typescript: tsc needs >=14.17, which is higher than the 14.14.0 that fs.rmSync needs. So on any Node.js where the next command in the chain could run at all, rmSync is present — there is no version where the clean step is what breaks first. More broadly you cannot lint, format, spellcheck or even commit in this repo on Node 10; npm run build:types was never runnable there, before or after this change (the scripts/build-types.js it replaces called the same fs.rmSync).
Raising engines.node to >=14.14.0 would be a breaking change for everyone consuming the package, to describe a constraint that applies only to contributors — so I'd rather not, and CI runs this job on lts/*.
Happy to switch the clean step to a tool CLI (del-cli, as webpack-dev-middleware and schema-utils do) if you'd prefer it on principle — I left it as node -e only because adding a dependency sits badly with a PR whose point is removing 32 of them.
Generated by Claude Code
…lag (#679) <!-- Thanks for submitting a pull request! Please provide enough information so that others can review your pull request. --> **Summary** Picks up @hey-amanthakur's #628, which went stale against `main` (conflicted, lint red) — original commit kept as co-author. Closes #628. `compileAliasOptions` builds the structure every resolve reads: the first-char buckets, the `useBuckets` switch that decides whether the hot path pays for a `Map.get`, and the per-option precomputed strings (`nameWithSlash`, `absolutePath`, `wildcardPrefix`/`wildcardSuffix`, `firstCharCode`, `arrayAlias`). Both bucket layouts resolve identically, so no resolve-level test can tell them apart — a regression in the layout, or in the conditions that turn bucketing off, is invisible until it silently stops aliasing something. These assert the compiled shape directly. Worth being precise about what this buys: `lib/AliasUtils.js` is already at 100% line coverage on `main` (333/333), so this is not a coverage patch. What it adds is behavioural pinning that line coverage does not give — declaration order inside a bucket (alias precedence depends on it), `useBuckets` off for a single first char and for an empty-prefix wildcard, the any-first-char entry (`firstCharCode === -1`) being kept out of the buckets, and the fact that a windows-style name normalizes `absolutePath` with a trailing `\` while `nameWithSlash` appends `/`, which is exactly why `aliasResolveHandler` compares against both. Changes from #628 as submitted: - Rebased onto current `main`; #628 also carried a `types.d.ts` reformat that an older Prettier had produced. That file is now a two-line stub over generated `types/` (#675), so the churn is gone rather than reapplied. - The `compileAliasOptions` tests used a `{ join: (filePath, ext) => filePath + ext }` stub, which made `absolutePath` come out as `/abs/path` and the test pinned that. The resolver builds it as `join(name, "_").slice(0, -1)`, so the real value is `/abs/path/` — the stub was pinning a shape the resolver never produces. These use a real resolver instead. - Added: declaration order within a bucket, a two-wildcard name (not treated as a wildcard alias), the windows `absolutePath` form, `arrayAlias`, and the `onlyModule` normalization. The `onlyModule` resolve tests now build one resolver per case through a helper rather than repeating the fixture three times, and cover a subpath (`real/sub/file`) as well as a relative request. **What kind of change does this PR introduce?** test — no `lib/` change, so resolution behaviour and performance are untouched. **Did you add tests for your changes?** That is the change: 13 tests in `test/alias.test.js`. The suite is 1629 tests, all passing, and `npm run lint` is clean across all five stages. **Does this PR introduce a breaking change?** No. **If relevant, what needs to be documented once your changes are merged or what have you already documented?** n/a — test-only, so no changeset either (nothing published changes). **Use of AI** Written with Claude Code, on top of @hey-amanthakur's original commit. It checked the assertions against the code rather than carrying them over: it measured `lib/AliasUtils.js` at 333/333 lines on `main` both with and without these tests, which is why the summary above does not claim a coverage gain; it ran `compileAliasOptions` with a real resolver to find that the submitted `absolutePath` assertion pinned the stub's output rather than the resolver's; and it confirmed the windows-name case is host-independent (`join` dispatches to `winNormalize` on the path type, not on the platform) before relying on it off Windows. Reviewed by me before opening. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_016aGHrb1YaEvaGwNELGEHjF --- _Generated by [Claude Code](https://claude.ai/code/session_016aGHrb1YaEvaGwNELGEHjF)_ <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Added coverage for alias configuration handling, including wildcard patterns, path normalization, and option flags. * Added tests verifying exact-match-only aliasing and subpath resolution behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: jhonsnow456 <thakuraman22july@gmail.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## enhanced-resolve@5.26.0 ### Minor Changes - Add experimental support for [Node.js package maps](https://nodejs.org/api/packages.html#package-maps) through a new `packageMap` option, which takes the path of the configuration file (or a `file:` `URL`) or an already-parsed `packages` object. When it is set, a bare specifier is resolved through the importing package's `dependencies` table and the target package's location is handed to the regular pipeline, instead of walking `node_modules`; relative and absolute requests and `node:` builtins are unaffected. Because several package entries may share one `url`, the package a request resolved into is exposed as `packageId` on the result and can be passed back in as `context.packageId` to resolve from that package unambiguously. Package maps are stability 1 (experimental) in Node.js, and this option tracks that specification and may change with it. (by [@alexander-akait](https://github.com/alexander-akait) in [#667](#667)) - Explain an `exports`/`imports` field whose conditions wrap subpaths, instead of failing with a message that points at the request. A field shaped like `{ "import": { ".": "./esm/index.js", "./*": "./esm/*.js" }, "require": "./build/bundle.js" }` is not supported by Node.js: the subpaths inside a condition are read as condition names, so they match nothing, and every request into the package failed as `"./foo" is not exported under the conditions [...]` — which reads as though the package forgot to export `./foo`. Such a failure now names the offending keys and shows the arrangement that works, with the subpaths at the top level and the conditions nested inside them. Resolution itself is unchanged: the diagnosis runs only on a request that has already failed, so nothing that resolves today starts failing, and a successful resolve does no extra work. Errors raised while processing either field also name the `package.json` they came from, which previously only appeared in the resolver log. (by [@alexander-akait](https://github.com/alexander-akait) in [#676](#676)) - Generate the published type declarations with TypeScript instead of `webpack/tooling`, which is no longer a dependency. Every name the package exported before is still exported, and `types.d.ts` is still the entry point, but the declarations themselves now live in `types/` and are emitted by `tsc` from the JSDoc in `lib/`. Two shapes follow the sources more closely than the previous generator did: the object form of `Plugin` no longer declares `this: Resolver` on `apply` (it is called as `plugin.apply(resolver)`, so `this` is the plugin), and the entries of `ResolveContext.stack` declare `name: string | undefined` rather than an optional `name`. Class fields that the old generator dropped, such as the cache backends on `CachedInputFileSystem`, are now part of the declarations. (by [@alexander-akait](https://github.com/alexander-akait) in [#675](#675)) ### Patch Changes - Size the ancestor path and segment arrays that `getPathsCached` keeps to what they actually hold: a `push`-built store keeps room for 17 entries while a path has a handful, and the cache holds these for the filesystem's lifetime. (by [@alexander-akait](https://github.com/alexander-akait) in [#681](#681)) Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
1 participant
Summary
webpack/toolingbundled every reachable type into a hand-rolledtypes.d.ts, which is a second type system to keep in sync with the JSDoc it is derived from — and it had drifted (it declaredthis: Resolveron the object form ofPlugin, butResolverFactorycallsplugin.apply(resolver), sothisis the plugin).tscnow emits the declarations intotypes/straight fromlib/, driven by nothing but the@typedefre-exports inlib/index.js, and the dependency is gone along with 32 transitive packages.types.d.tsstays the entry point and re-exports them. Generation is abuild:typesscript field calling the tools' own CLIs — no wrapper script. This matcheswatchpack,schema-utils,mini-css-extract-plugin,terser-webpack-pluginandcopy-webpack-plugin, which all ship tsc-generated per-file declarations.What kind of change does this PR introduce?
refactor (one of the commits is ci).
Did you add tests for your changes?
No new test files, and none are needed:
types/is generated and committed, and the newCheck typesCI step regenerates and diffs it, so dropping a@typedeffromlib/index.jschanges a committed file and fails the build. The 1616 existing tests are unchanged and pass.Does this PR introduce a breaking change?
No — every name exported before is still exported. Two declarations follow the sources more closely than the old generator did: the object form of
Pluginno longer claimsthis: Resolveronapply(implementations stay assignable either way), andResolveContext.stackentries declarename: string | undefinedrather than an optionalname. Class fields the old generator dropped, such as the cache backends onCachedInputFileSystem, are now part of the declarations.If relevant, what needs to be documented once your changes are merged or what have you already documented?
n/a — the changeset covers the changelog.
Use of AI
Written with Claude Code. It made the change and verified the export surface three ways rather than asserting it:
lib/index.jsdeclares 19 typedefs,types/index.d.tsexports exactly those 19, and they are exactly the 19 nameswebpack/toolingexported at332d999— nothing lost, nothing added; deleting theResolveRequesttypedef drops it from the declarations entirely and surfaces as a modifiedtypes/index.d.ts, which is what the CI step fails on; and a consumer installed fromnpm packimports all 19, including the four plugin classes in type position, and compiles. The CI check was also tested against a stale checkout, a modified JSDoc comment and a deleted source file. Reviewed by me before opening.🤖 Generated with Claude Code
https://claude.ai/code/session_016aGHrb1YaEvaGwNELGEHjF
Summary by CodeRabbit
New Features
Bug Fixes
Chores