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: Add rule react-no-unsafe-timeout #14650

Merged
merged 3 commits into from
Mar 27, 2019

Conversation

aduth
Copy link
Member

@aduth aduth commented Mar 27, 2019

This pull request seeks to introduce a new ESLint rule @wordpress/react-no-unsafe-timeout. In brief, it is meant to enforce the anti-pattern of setTimeout use in components which withSafeTimeout exists to address.

From the included rule documentation:

setTimeout in a component must be cancelled when the component is unmounted. This rule disallows references to the setTimeout global which occur in a component, and which are not assigned to a variable. A variable assignment is considered an acceptable use on the assumption that the timeout ID will be later referenced for cancellation via clearTimeout.

Consider using the withSafeTimeout higher-order component from the @wordpress/compose module.

In introducing the rule, it also resolves a few existing offending occurrences in code:

  • packages/block-editor/src/components/rich-text/index.js
  • packages/block-editor/src/components/url-input/index.js
  • packages/components/src/form-token-field/suggestions-list.js

Testing Instructions:

Verify there are no errors when running npm run lint-js.

Verify unit tests pass running npm run test-unit

@aduth aduth added [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. [Tool] ESLint plugin /packages/eslint-plugin labels Mar 27, 2019
@aduth aduth requested a review from ellatrix March 27, 2019 01:46
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.

Nice one, I'm starting to get comfortable with reviewing those ESLint rules. I like that all recent efforts are focused on preventing common mistakes rather than code style polishing 💯

code: `class MyComponent extends Component { componentDidMount() { this.props.setTimeout(); } }`,
},
{
code: `class MyComponent extends Component { componentDidMount() { this.timeoutId = setTimeout(); } }`,
Copy link
Member

Choose a reason for hiding this comment

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

It still can be invalid (not canceled when the component gets unmount) but I think it's fine to assume good intents and keep implementation reasonable :)

Copy link
Member Author

Choose a reason for hiding this comment

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

It still can be invalid (not canceled when the component gets unmount) but I think it's fine to assume good intents and keep implementation reasonable :)

Right, the thinking was: You'd only ever care about the value produced by setTimeout if you intended to use it in clearTimeout (the return value is described as such). There's no guarantee here that this actually occurs, or that it occurs specifically in componentWillUnmount. But it's also the only option for considering "safe" in a generic sense where someone may want to use this rule, but wouldn't use withSafeTimeout from @wordpress/compose.

@aduth aduth merged commit ba64d41 into master Mar 27, 2019
@aduth aduth deleted the add/eslint-react-no-unsafe-timeout branch March 27, 2019 13:35
@youknowriad youknowriad added this to the 5.4 (Gutenberg) milestone Mar 29, 2019
@nerrad
Copy link
Contributor

nerrad commented Mar 31, 2019

@aduth - I'm curious, why is there no CHANGELOG.md entry for this new rule added?

@aduth
Copy link
Member Author

aduth commented Apr 1, 2019

@nerrad Oversight. I'll open a pull request shortly. Thanks for noting it.

@nerrad
Copy link
Contributor

nerrad commented Apr 1, 2019

I figured :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Tool] ESLint plugin /packages/eslint-plugin [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants