Add more precise types in the JSDoc #818

stof · 2020-08-03T23:21:56Z

These are the type improvements coming from my investigation in #816 (and then pushed until the end rather than stopping at a few methods for the experiment)

stof · 2020-08-04T08:36:08Z

The failing tests seem unrelated to my PR. I'm changing only comments

stof · 2020-08-04T08:41:15Z

hmm, looks like the jsdoc parser of eslint does not like the object types defining properties. And given that the valid-jsdoc rule is deprecated and the corresponding parser is unmaintained, it is unlikely to be fixed.

@Lyrkan @weaverryan should we disable that rule ?

stof · 2020-08-04T08:44:02Z

Or maybe we should migrate to https://github.com/gajus/eslint-plugin-jsdoc

Lyrkan

Sorry for not looking at this sooner, those are some great changes :)

@Lyrkan @weaverryan should we disable that rule ?
Or maybe we should migrate to gajus/eslint-plugin-jsdoc

We could try gajus/eslint-plugin-jsdoc but I wouldn't mind disabling valid-jsdoc entirely either tbh.

weaverryan · 2020-12-03T13:29:11Z

My preference would be to remove our valid-jsdoc and try https://github.com/gajus/eslint-plugin-jsdoc - it looks high quality, and i do kinda like our very-complete (and validated with eslint) jsdoc.

stof · 2020-12-03T17:03:49Z

@Lyrkan @weaverryan I made the migration to eslint-plugin-jsdoc (and I fixed all the extra things it reports, which were not detected by valid-jsdoc)

stof · 2020-12-03T17:20:40Z

The failure for the job Lowest versions of the dependencies is unrelated to my change. that's also broken in the main branch.

Kocal · 2024-08-26T22:42:30Z

Hey, what about using TypeScript to check types?
No transpilation, just checking types on .js files with allowJs/checkJs options, some JSDoc, and tsc --noEmit?

stof · 2024-08-30T09:28:32Z

@Kocal using typescript to check types on our own codebase could be a next step after that. But it would require having types defined properly for all our code, while this PR was focusing on what impacts our public API.
The goal was to allow downstream projects to use typescript to check types on their webpack.config.js file.

I should definitely revive that PR as it would be a nice addition.

Kocal · 2024-08-30T09:32:43Z

@stof you're right, that makes sense.

I should definitely revive that PR as it would be a nice addition.

Totally! :)

stof · 2024-09-02T11:02:25Z

I updated the PR (it will conflict with #1313 though).

I used object for the configuration in a bunch of cases to keep our types free from adding new dependencies (many of our features rely on optional dependencies, but the types of the main public API cannot). There's also a few of our dependencies that don't provide type declarations for their configuration (css-loader and style-loader for instance).

I also kept object for the configuration of SplitChunksPlugin because the types used for the options of this plugin are not exported. I was able to write a type that would work for type checking our project but would prevent using --emitDeclarationOnly due to the usage of private types (and so would block #816)

This documents the value type for arrays of strings in arguments.

webpack provides type definitions for their whole API, so we can reference their type rather than specifying a generic "object" type. Referencing the webpack types was already done for the returned configuration.

Most of them are documented as receiving an object and returning an object or void. But some callbacks dealing with parts of the webpack configuration itself have a more precise type for the options object.

This brings the list of supported options in the type definitions with their type, which lets IDEs provide autocompletion for them.

Kocal

Wow 🤩

That's will definitely increase DX for end-users and for us, just a minor comment tho

Kocal · 2024-09-03T05:31:26Z

lib/utils/package-up.js

@@ -16,13 +16,17 @@ const { fileURLToPath } = require('url');
 /**
 * Inlined version of the package "package-up" (ESM only).
 *
- * @param {string} cwd The directory to start searching from.
+ * @param {{cwd: string}} options The directory to start searching from.


Shouldn't we keep the previous syntax? Here the description "The directory (...)" refers to the options object, and not to cwd right?

Or we can remove the description, since it's super internal, I don't really have a strong opinion on this.

The previous syntax describes a cwd argument which does not exist (there is a cwd property in the first argument instead).

I updated the JSDoc to use the alternative syntax allowing to put a description on properties (this was already used later in the file)

Lyrkan reviewed Sep 2, 2020

View reviewed changes

weaverryan changed the base branch from master to main November 18, 2020 18:17

stof force-pushed the improve_js_doc branch from 7f87884 to 17ce5fd Compare December 3, 2020 15:27

stof requested review from Lyrkan and weaverryan December 5, 2020 14:52

stof force-pushed the improve_js_doc branch from 3462433 to 609fd3d Compare April 21, 2021 12:41

stof mentioned this pull request Aug 30, 2024

Migrate to eslint-plugin-jsdoc #1311

Merged

stof force-pushed the improve_js_doc branch 2 times, most recently from 5088cca to f41dda2 Compare September 2, 2024 10:44

stof mentioned this pull request Sep 2, 2024

Drop css-minimizer-webpack-plugin 5 support, only css-minimizer-webpack-plugin 7 is supported #1320

Closed

stof added 4 commits September 2, 2024 18:35

Add more precise JSDoc for arrays

bd2e616

This documents the value type for arrays of strings in arguments.

Add more precise types for arguments forwarded to webpack

ee6097c

webpack provides type definitions for their whole API, so we can reference their type rather than specifying a generic "object" type. Referencing the webpack types was already done for the returned configuration.

Add more precise JSDoc for the options callbacks

b31b345

Most of them are documented as receiving an object and returning an object or void. But some callbacks dealing with parts of the webpack configuration itself have a more precise type for the options object.

Add more precise JSDoc for Encore options

193d10e

This brings the list of supported options in the type definitions with their type, which lets IDEs provide autocompletion for them.

stof force-pushed the improve_js_doc branch from f41dda2 to bbf7c8f Compare September 2, 2024 16:49

stof requested review from Kocal and removed request for weaverryan and Lyrkan September 2, 2024 16:49

Kocal approved these changes Sep 3, 2024

View reviewed changes

Add more precise types for the package-up utility

876de53

Fix typos in JSDoc

4485ee4

stof force-pushed the improve_js_doc branch from bbf7c8f to 4485ee4 Compare September 3, 2024 07:40

stof merged commit 903aaa6 into symfony:main Sep 3, 2024
28 checks passed

stof deleted the improve_js_doc branch September 3, 2024 08:21

This was referenced Sep 3, 2024

Remove no-op argument #1325

Merged

Fix compat with configuring the devserver server as a string #1327

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add more precise types in the JSDoc #818

Add more precise types in the JSDoc #818

stof commented Aug 3, 2020

stof commented Aug 4, 2020

stof commented Aug 4, 2020

stof commented Aug 4, 2020

Lyrkan left a comment

weaverryan commented Dec 3, 2020

stof commented Dec 3, 2020

stof commented Dec 3, 2020

Kocal commented Aug 26, 2024 •

edited

Loading

stof commented Aug 30, 2024 •

edited

Loading

Kocal commented Aug 30, 2024

stof commented Sep 2, 2024

Kocal left a comment

Kocal Sep 3, 2024

stof Sep 3, 2024

stof Sep 3, 2024

Add more precise types in the JSDoc #818

Add more precise types in the JSDoc #818

Conversation

stof commented Aug 3, 2020

stof commented Aug 4, 2020

stof commented Aug 4, 2020

stof commented Aug 4, 2020

Lyrkan left a comment

Choose a reason for hiding this comment

weaverryan commented Dec 3, 2020

stof commented Dec 3, 2020

stof commented Dec 3, 2020

Kocal commented Aug 26, 2024 • edited Loading

stof commented Aug 30, 2024 • edited Loading

Kocal commented Aug 30, 2024

stof commented Sep 2, 2024

Kocal left a comment

Choose a reason for hiding this comment

Kocal Sep 3, 2024

Choose a reason for hiding this comment

stof Sep 3, 2024

Choose a reason for hiding this comment

stof Sep 3, 2024

Choose a reason for hiding this comment

Kocal commented Aug 26, 2024 •

edited

Loading

stof commented Aug 30, 2024 •

edited

Loading