Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ESLint Plugin: Continue considering unused variables after encountering exception #21354

Merged
merged 4 commits into from Apr 10, 2020

Conversation

aduth
Copy link
Member

@aduth aduth commented Apr 2, 2020

Previously: #16737

This pull request seeks to fix an issue with the custom ESLint rule @wordpress/no-unused-vars-before-return, where an exception identified by the excludePattern will cause all other candidate issues to be skipped, therefore allowing errors to be inadvertently introduced. Notably, this would occur in Gutenberg when React hooks are present. And it was more pronounced because React hooks should always occur at the very beginning of a component's render implementation.

Implementation notes:

At the most basic, the issue is that a continue; should have been used to skip an exception when iterating candidate issues, but a return; was used instead, thus aborting checks of all other candidates of the loop.

Fixes to current issues are included in these changes.

Testing Instructions:

Ensure unit tests pass:

npm run test-unit packages/eslint-plugin/rules/__tests__/no-unused-vars-before-return.js

Ensure lint passes:

npm run lint-js

@aduth aduth added [Type] Bug An existing feature does not function as intended [Package] ESLint plugin /packages/eslint-plugin labels Apr 2, 2020
@aduth aduth added the Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code label Apr 2, 2020
@github-actions
Copy link

github-actions bot commented Apr 2, 2020

Size Change: -7 B (0%)

Total Size: 903 kB

Filename Size Change
build/block-editor/index.js 104 kB +5 B (0%)
build/block-library/index.js 112 kB -13 B (0%)
build/edit-post/index.js 93.5 kB +1 B
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.02 kB 0 B
build/annotations/index.js 3.62 kB 0 B
build/api-fetch/index.js 4.01 kB 0 B
build/autop/index.js 2.82 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.24 kB 0 B
build/block-directory/style-rtl.css 760 B 0 B
build/block-directory/style.css 760 B 0 B
build/block-editor/style-rtl.css 10.2 kB 0 B
build/block-editor/style.css 10.2 kB 0 B
build/block-library/editor-rtl.css 7.22 kB 0 B
build/block-library/editor.css 7.22 kB 0 B
build/block-library/style-rtl.css 7.15 kB 0 B
build/block-library/style.css 7.16 kB 0 B
build/block-library/theme-rtl.css 683 B 0 B
build/block-library/theme.css 685 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 57.7 kB 0 B
build/components/index.js 198 kB 0 B
build/components/style-rtl.css 16.6 kB 0 B
build/components/style.css 16.5 kB 0 B
build/compose/index.js 6.66 kB 0 B
build/core-data/index.js 11.1 kB 0 B
build/data-controls/index.js 1.25 kB 0 B
build/data/index.js 8.43 kB 0 B
build/date/index.js 5.47 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 569 B 0 B
build/dom/index.js 3.1 kB 0 B
build/edit-navigation/index.js 3.1 kB 0 B
build/edit-navigation/style-rtl.css 279 B 0 B
build/edit-navigation/style.css 280 B 0 B
build/edit-post/style-rtl.css 12.3 kB 0 B
build/edit-post/style.css 12.3 kB 0 B
build/edit-site/index.js 10.4 kB 0 B
build/edit-site/style-rtl.css 5.02 kB 0 B
build/edit-site/style.css 5.02 kB 0 B
build/edit-widgets/index.js 7.53 kB 0 B
build/edit-widgets/style-rtl.css 3.74 kB 0 B
build/edit-widgets/style.css 3.73 kB 0 B
build/editor/editor-styles-rtl.css 428 B 0 B
build/editor/editor-styles.css 431 B 0 B
build/editor/index.js 43.6 kB 0 B
build/editor/style-rtl.css 3.49 kB 0 B
build/editor/style.css 3.49 kB 0 B
build/element/index.js 4.64 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.29 kB 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.56 kB 0 B
build/is-shallow-equal/index.js 711 B 0 B
build/keyboard-shortcuts/index.js 2.51 kB 0 B
build/keycodes/index.js 1.91 kB 0 B
build/list-reusable-blocks/index.js 3.12 kB 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/media-utils/index.js 5.28 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.4 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/plugins/index.js 2.67 kB 0 B
build/primitives/index.js 1.49 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.84 kB 0 B
build/rich-text/index.js 14.8 kB 0 B
build/server-side-render/index.js 2.67 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.28 kB 0 B
build/url/index.js 4.01 kB 0 B
build/viewport/index.js 1.84 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

Looks good, let’s rebase and land it 👍

packages/eslint-plugin/CHANGELOG.md Outdated Show resolved Hide resolved
@aduth aduth force-pushed the fix/no-unused-vars-before-return-skips-all branch from 0fb7623 to fd56b48 Compare April 10, 2020 19:18
@aduth aduth merged commit e3580a6 into master Apr 10, 2020
@aduth aduth deleted the fix/no-unused-vars-before-return-skips-all branch April 10, 2020 20:18
@github-actions github-actions bot added this to the Gutenberg 7.9 milestone Apr 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code [Package] ESLint plugin /packages/eslint-plugin [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants