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

Remove no-render-return-value from recommended #2259

Closed
wants to merge 1 commit into from

Conversation

bz2
Copy link

@bz2 bz2 commented May 3, 2019

This rules is about a potential future change to a ReactDOM interface
not any actual problem, and is the slowest plugin:react/recommended
by a factor of about 3.

Timings from one of our projects:

Rule                           | Time (ms) | Relative
:------------------------------|----------:|--------:
import/namespace               |  5505.442 |    19.3%
indent                         |  4169.809 |    14.6%
react/no-render-return-value   |  3402.119 |    11.9%
import/named                   |  1252.616 |     4.4%
react/no-deprecated            |  1165.668 |     4.1%
react/display-name             |  1126.994 |     4.0%
react/no-direct-mutation-state |  1109.049 |     3.9%
react/sort-comp                |   691.149 |     2.4%
react/no-string-refs           |   680.091 |     2.4%
import/default                 |   622.367 |     2.2%

Removing the rule does save 4s from a 60s run.

This rules is about a potential future change to a ReactDOM interface
not any actual problem, and is the slowest `plugin:react/recommended`
by a factor of about 3.
@bz2
Copy link
Author

bz2 commented May 3, 2019

Appveyor failed but only one platform and looks spurious:

Environment: nodejs_version=8; Platform: x64
Build started
Fetching repository commit (873816e)...An error occurred while sending the request.

@ljharb
Copy link
Member

ljharb commented May 3, 2019

I don't think "slowest" should be a disqualifier from whether it's recommended; it's a lint run in CI.

@ljharb
Copy link
Member

ljharb commented May 3, 2019

Separately, this is an actual problem for future migrations, so it seems useful to keep recommended.

As for it being slow, it doesn't seem like it should be - making it faster would be great tho!

@yannickcr
Copy link
Member

Closing since performance problem was likely fixed in #2264

@yannickcr yannickcr closed this Jun 23, 2019
@bz2
Copy link
Author

bz2 commented Jun 23, 2019

I still think this is not a good rule to enable by default, but thanks @golopot for making it less painful.

There's no harm in code just failing when an actual migration happens. I have two instances of ReactDOM.render() in my entire codebase, this isn't a thing it makes sense to check every line in the project for.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants