docs(NormalModule): enhance JSDocs and typings for loaderContext#20888
Conversation
|
231ca88 to
7d67d6a
Compare
| /** @type {AnyLoaderContext} */ | ||
| (loaderContext) | ||
| ); | ||
| getOptions: /** @type {LoaderContext<EXPECTED_ANY>["getOptions"]} */ ( |
There was a problem hiding this comment.
Please move it to top of getOptions no using as
There was a problem hiding this comment.
I actually tried doing this before the final commit, but the TS compiler throws an error (Target signature provides too few arguments) because getOptions is overloaded in LoaderContext.d.ts.
If we don't want to use casting, we can use one of these two ideas:
- Merge the overloads into a single signature with an optional parameter like:
getOptions(schema?: Schema): OptionsType;. - Keep the overloads, but add a default parameter to the runtime method
getOptions: (schema = undefined) => { ... }.
any other suggestion?
There was a problem hiding this comment.
it means we need to fix types in this case
There was a problem hiding this comment.
Yeah, if we use the first option.
However, I think of others overload signature functions that has completely different parameters and how we can handle this.
That's why I asked for suggestions.
There was a problem hiding this comment.
We should keep overloads, because they help to make less bugs, so feel free to
Keep the overloads, but add a default parameter to the runtime method getOptions: (schema = undefined) => { ... }.
There was a problem hiding this comment.
Done.
Added an EXPECTED_ANY type for schema cause it threw an error when accessing schema.title later in the function.
/** @type {LoaderContext<EXPECTED_ANY>["getOptions"]} */
getOptions: (/** @type {EXPECTED_ANY} */ schema = undefined) => {
const loader = this.getCurrentLoader(
/** @type {AnyLoaderContext} */
(loaderContext)
);
let ...7d67d6a to
e6f2c27
Compare
|
@moshams272 sorry for delay, can you rebase, thanks |
e6f2c27 to
b262323
Compare
NP, Done! |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #20888 +/- ##
==========================================
- Coverage 91.58% 91.56% -0.03%
==========================================
Files 573 573
Lines 59277 59491 +214
Branches 16012 16065 +53
==========================================
+ Hits 54289 54471 +182
- Misses 4988 5020 +32
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Merging this PR will degrade performance by 22.53%
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing Footnotes
|
|
Need fix lint |
… and LoaderContext
b262323 to
e042e5a
Compare
|
Lint failed |
|
This PR is packaged and the instant preview is available (7f891b0). Install it locally:
npm i -D webpack@https://pkg.pr.new/webpack@7f891b0
yarn add -D webpack@https://pkg.pr.new/webpack@7f891b0
pnpm add -D webpack@https://pkg.pr.new/webpack@7f891b0 |
Summary
This PR enhances and standardizes the JSDoc typings for the
loaderContextmethods inlib/NormalModule.js.JSDoc descriptions were moved to
declarations/LoaderContext.d.ts, andlib/NormalModule.jsnow uses type references to avoid duplication and keep the runtime code clean.Part Of Issue: webpack/webpack-doc-kit#88
What kind of change does this PR introduce?
docs
Did you add tests for your changes?
No. This PR only updates JSDoc comments and type annotations
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?
Nothing.
Use of AI
Yes. I used it to help format the JSDoc comments to match Webpack's internal coding style. However, all types and logic were manually traced and verified against the actual Webpack source code before submission.