Skip to content

Conversation

@noahtallen
Copy link
Contributor

@noahtallen noahtallen commented Jan 19, 2023

Proposed Changes

Separating this from #72245 to simplify things. Updates eslint-plugin-jsdoc across the repo to properly support Node 18. cc @anomiex

The initial update introduced two problems:

  1. Firstly, JSDoc wanted to replace Object with object.
  2. Secondly, there were a lot of issues with require-returns check. It seemed to have gotten a lot smarter :)

The first fix is easy -- just add a setting which allows both Object and object. I don't see a big reason to enforce one over the other at this point, especially since we'd rather use Typescript.

The second one just meant going through several files and fixing the return type. Almost every error was caused by functions like this:

/**
 * @returns {string}
 */
function foo( x ) {
  if ( x ) {
    return "bar";
  }
}

This is obviously problematic, because the real return type is string|undefined. So I just made that adjustment in several files.

There are a few more issues reported by the main linter build below, but not enough to fail the build. I did find weird behavior in the eslint jsdoc lib, which I reported here: gajus/eslint-plugin-jsdoc#949

Testing Instructions

CI, and verify that changes make sense. The full eslint build is running here: https://teamcity.a8c.com/buildConfiguration/calypso_CheckCodeStyle/9329626

@noahtallen noahtallen requested a review from a team as a code owner January 19, 2023 01:12
@noahtallen noahtallen requested a review from a team January 19, 2023 01:12
@noahtallen noahtallen requested a review from a team as a code owner January 19, 2023 01:12
@matticbot matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jan 19, 2023
@github-actions
Copy link

github-actions bot commented Jan 19, 2023

@matticbot
Copy link
Contributor

This PR modifies the release build for happy-blocks

To test your changes on WordPress.com, run install-plugin.sh happy-blocks update-jsdoc-eslint-plugin on your sandbox.

To deploy your changes after merging, see the documentation: PCYsg-r7r-p2

@noahtallen noahtallen mentioned this pull request Jan 19, 2023
2 tasks
@noahtallen noahtallen self-assigned this Jan 19, 2023
@matticbot
Copy link
Contributor

This PR modifies the release build for editing-toolkit

To test your changes on WordPress.com, run install-plugin.sh editing-toolkit update-jsdoc-eslint-plugin on your sandbox.

To deploy your changes after merging, see the documentation: PCYsg-mMA-p2

@matticbot
Copy link
Contributor

This PR modifies the release build for odyssey-stats

To test your changes on WordPress.com, run install-plugin.sh odyssey-stats update-jsdoc-eslint-plugin on your sandbox.

To deploy your changes after merging, see the documentation: PejTkB-3N-p2

@matticbot
Copy link
Contributor

matticbot commented Jan 19, 2023

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

Sections (~1 bytes removed 📉 [gzipped])

Details
name    parsed_size           gzip_size
media         +18 B  (+0.0%)       +6 B  (+0.0%)
reader        -16 B  (-0.0%)       -7 B  (-0.0%)

Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to.

Async-loaded Components (~1 bytes removed 📉 [gzipped])

Details
name                                                     parsed_size           gzip_size
async-load-calypso-post-editor-media-modal                     +18 B  (+0.0%)       +6 B  (+0.0%)
async-load-calypso-post-editor-editor-media-modal              +18 B  (+0.0%)       +6 B  (+0.0%)
async-load-calypso-blocks-support-article-dialog-dialog        -16 B  (-0.0%)       -7 B  (-0.0%)
async-load-automattic-help-center                              -16 B  (-0.0%)       -7 B  (-0.0%)

React components that are loaded lazily, when a certain part of UI is displayed for the first time.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

@noahtallen noahtallen force-pushed the update-jsdoc-eslint-plugin branch from 3db3471 to 99b49a0 Compare January 19, 2023 01:31
Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this 🙌

Left a few suggestions and questions, let me know what you think.

FWIW I think it looks great already!

@noahtallen
Copy link
Contributor Author

BTW, the reason there are so many oddities in the jsdoc eslint plugin is because they actually fixed a bug with the @returns rule! Before the update, the rule would pass as long as a single return statement existed. Now, they've fixed it so that it checks every branch of the code. This is definitely better overall, because it forces us to use correct types in more places. But there do seem to be one or two bugs with this new behavior :)

Bug reports:

gajus/eslint-plugin-jsdoc#950

gajus/eslint-plugin-jsdoc#949

@noahtallen noahtallen requested review from a team and worldomonation as code owners January 19, 2023 20:22
@noahtallen noahtallen requested a review from a team January 19, 2023 20:22
@noahtallen noahtallen requested a review from a team as a code owner January 19, 2023 20:22
@noahtallen noahtallen force-pushed the update-jsdoc-eslint-plugin branch from 503c5a7 to 7d167ec Compare January 19, 2023 20:23
@noahtallen noahtallen removed request for a team January 19, 2023 20:24
@noahtallen noahtallen requested review from a team and removed request for worldomonation January 19, 2023 20:24
Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great 👍 Thanks @noahtallen! 🚀

I've committed a couple of last-minute fixes if you don't mind.

@tyxla
Copy link
Member

tyxla commented Jan 20, 2023

Oh, I just saw #72348. Feel free to delete that commit!

@noahtallen
Copy link
Contributor Author

noahtallen commented Jan 20, 2023

Wow, they already released a fix for one of my bug reports! I updated to include that version, which let us remove the weird *|undefined definitions we had to add originally.

@noahtallen noahtallen force-pushed the update-jsdoc-eslint-plugin branch from f42259f to 3f3340c Compare January 20, 2023 22:17
@noahtallen noahtallen merged commit 9796ddf into trunk Jan 20, 2023
@noahtallen noahtallen deleted the update-jsdoc-eslint-plugin branch January 20, 2023 22:30
@github-actions github-actions bot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jan 20, 2023
autumnfjeld pushed a commit that referenced this pull request Jan 23, 2023
* Update eslint-plugin-jsdoc peer dep

* fix jsdoc/require-returns-check rule

* Fix type in media modal component

* Fix jsdoc/check-param-names

* Fix more instances of jsdoc return issue by returning null from react components

Co-authored-by: Marin Atanasov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants