fix: improve types for NormalModule#20721
Conversation
|
|
This PR is packaged and the instant preview is available (c7da642). Install it locally:
npm i -D webpack@https://pkg.pr.new/webpack@c7da642
yarn add -D webpack@https://pkg.pr.new/webpack@c7da642
pnpm add -D webpack@https://pkg.pr.new/webpack@c7da642 |
There was a problem hiding this comment.
Pull request overview
This PR refines webpack’s type surface around NormalModule by introducing generics for parser/generator types and updating related APIs/hooks to reference the parameterized NormalModule, with the goal of enabling future module-type-specific typing improvements.
Changes:
- Make
NormalModulegeneric over parser/parserOptions and generator/generatorOptions, and thread that through manyNormalModule-consuming type signatures. - Add JSDoc generics to
NormalModuleFactory.getParser/createParser/getGenerator/createGeneratorand adjust implementations with casts. - Specialize
CssModule’s relationship toNormalModulewith concrete parser/generator types (JS JSDoc +types.d.ts).
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
types.d.ts |
Introduces generic NormalModule and updates numerous signatures/hooks to use the parameterized form. |
lib/NormalModuleFactory.js |
Adds JSDoc templates for parser/generator factory methods and returns typed casts. |
lib/NormalModule.js |
Adds generics to NormalModule and its create-data typedef; updates internal casts accordingly. |
lib/CssModule.js |
Adds typed @extends and create-data typing to bind CssModule to specific parser/generator types. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * @property {PO | undefined} parserOptions the options of the parser used | ||
| * @property {G} generator the generator used | ||
| * @property {GO | undefined} generatorOptions the options of the generator used |
There was a problem hiding this comment.
In the NormalModuleCreateData typedef, parserOptions/generatorOptions are modeled as {PO | undefined} and {GO | undefined} instead of being marked optional (e.g. {PO=} / {GO=} like other typedef object properties in this repo). This can force object literals to explicitly include these properties (often as undefined) under checkJs/strict and makes the typedef inconsistent with established optional-property JSDoc style here.
| * @property {PO | undefined} parserOptions the options of the parser used | |
| * @property {G} generator the generator used | |
| * @property {GO | undefined} generatorOptions the options of the generator used | |
| * @property {PO=} parserOptions the options of the parser used | |
| * @property {G} generator the generator used | |
| * @property {GO=} generatorOptions the options of the generator used |
| this.parserOptions = | ||
| /** @type {PO} */ | ||
| (unsafeCacheData.parserOptions); | ||
| this.parser = | ||
| /** @type {P} */ | ||
| (normalModuleFactory.getParser(this.type, this.parserOptions)); | ||
| this.generatorOptions = | ||
| /** @type {GO} */ | ||
| (unsafeCacheData.generatorOptions); | ||
| this.generator = | ||
| /** @type {G} */ | ||
| (normalModuleFactory.getGenerator(this.type, this.generatorOptions)); |
There was a problem hiding this comment.
In _restoreFromUnsafeCache, the casts force unsafeCacheData.parserOptions/generatorOptions to PO/GO, but these values can be undefined (and the instance fields are typed as PO | undefined / GO | undefined). Casting to PO | undefined / GO | undefined keeps the JSDoc types accurate under checkJs/strict and avoids masking incorrect assumptions about these fields always being present.
| @@ -1376,21 +1377,24 @@ If changing the source code is not an option there is also a resolve options cal | |||
| cache.set(parserOptions, parser); | |||
| } | |||
|
|
|||
| return parser; | |||
| return /** @type {P} */ (parser); | |||
| } | |||
|
|
|||
| /** | |||
| * @template {Parser} [P=Parser] | |||
| * @param {string} type type | |||
| * @param {ParserOptions} parserOptions parser options | |||
| * @returns {Parser} parser | |||
| * @returns {P} parser | |||
| */ | |||
| createParser(type, parserOptions = {}) { | |||
| parserOptions = mergeGlobalOptions( | |||
| this._globalParserOptions, | |||
| type, | |||
| parserOptions | |||
| ); | |||
| const parser = this.hooks.createParser.for(type).call(parserOptions); | |||
| const parser = | |||
| /** @type {P} */ | |||
| (this.hooks.createParser.for(type).call(parserOptions)); | |||
| if (!parser) { | |||
| throw new Error(`No parser registered for ${type}`); | |||
| } | |||
| @@ -1399,9 +1403,10 @@ If changing the source code is not an option there is also a resolve options cal | |||
| } | |||
|
|
|||
| /** | |||
| * @template {Generator} [G=Generator] | |||
| * @param {string} type type of generator | |||
| * @param {GeneratorOptions} generatorOptions generator options | |||
| * @returns {Generator} generator | |||
| * @returns {G} generator | |||
| */ | |||
| getGenerator(type, generatorOptions = EMPTY_GENERATOR_OPTIONS) { | |||
| let cache = this.generatorCache.get(type); | |||
| @@ -1418,23 +1423,24 @@ If changing the source code is not an option there is also a resolve options cal | |||
| cache.set(generatorOptions, generator); | |||
| } | |||
|
|
|||
| return generator; | |||
| return /** @type {G} */ (generator); | |||
| } | |||
|
|
|||
| /** | |||
| * @template {Generator} [G=Generator] | |||
| * @param {string} type type of generator | |||
| * @param {GeneratorOptions} generatorOptions generator options | |||
| * @returns {Generator} generator | |||
| * @returns {G} generator | |||
| */ | |||
| createGenerator(type, generatorOptions = {}) { | |||
| generatorOptions = mergeGlobalOptions( | |||
| this._globalGeneratorOptions, | |||
| type, | |||
| generatorOptions | |||
| ); | |||
| const generator = this.hooks.createGenerator | |||
| .for(type) | |||
| .call(generatorOptions); | |||
| const generator = | |||
| /** @type {G} */ | |||
| (this.hooks.createGenerator.for(type).call(generatorOptions)); | |||
| if (!generator) { | |||
There was a problem hiding this comment.
The @template-based return typing for getParser/createParser and getGenerator/createGenerator lets callers pick an arbitrary subtype for P/G, but the runtime selection is based only on the type string. This is effectively a typed cast and can hide real type mismatches (e.g., requesting CssParser for a JS module type). Consider returning the base Parser/Generator types here (or deriving the generic from type via a mapping) rather than letting callers specify it freely.
Merging this PR will degrade performance by 31.73%
Performance Changes
Comparing 
|
Summary
this is a part of future improvement when we will have right parser and parser options, generator and generator options in parse/generate step (and so the right module), also it will help in future
AssetModule/JsonModuleand etc improvementsAlso in future we will split builtMeta and builtInfo by module types, now we store all properties into two types, but for example js module will never have css related properties
What kind of change does this PR introduce?
fix
Did you add tests for your changes?
Existing
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?
Noting
Use of AI
No