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!

golopot added a commit to golopot/eslint-plugin-react that referenced this pull request May 6, 2019
This commit likely fixes the performace issue noted in jsx-eslint#2259.
ljharb pushed a commit to golopot/eslint-plugin-react that referenced this pull request May 6, 2019
This commit likely fixes the performace issue noted in jsx-eslint#2259.
golopot added a commit to golopot/eslint-plugin-react that referenced this pull request May 16, 2019
This commit likely fixes the performace issue noted in jsx-eslint#2259.
@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.

kodiakhq bot pushed a commit to mcansh/connection that referenced this pull request Jul 26, 2019
## The devDependency [eslint-plugin-react](https://github.com/yannickcr/eslint-plugin-react) was updated from `7.13.0` to `7.14.0`.
This version is **not covered** by your **current version range**.

If you don’t accept this pull request, your project will work just like it did before. However, you might be missing out on a bunch of new features, fixes and/or performance improvements from the dependency update.

---

<details>
<summary>Release Notes for v7.14.0</summary>

<h3>Added</h3>
<ul>
<li>Add <code>jsx-curly-newline</code> rule (<a href="https://urls.greenkeeper.io/yannickcr/eslint-plugin-react/issues/1493" data-hovercard-type="issue" data-hovercard-url="/jsx-eslint/eslint-plugin-react/issues/1493/hovercard">#1493</a> <a class="user-mention" data-hovercard-type="user" data-hovercard-url="/hovercards?user_id=16285118" data-octo-click="hovercard-link-click" data-octo-dimensions="link_type:self" href="https://urls.greenkeeper.io/golopot">@golopot</a>)</li>
<li>Add support for nested destructuring to <code>prop-types</code> (<a href="https://urls.greenkeeper.io/yannickcr/eslint-plugin-react/issues/296" data-hovercard-type="issue" data-hovercard-url="/jsx-eslint/eslint-plugin-react/issues/296/hovercard">#296</a> <a href="https://urls.greenkeeper.io/yannickcr/eslint-plugin-react/issues/1422" data-hovercard-type="issue" data-hovercard-url="/jsx-eslint/eslint-plugin-react/issues/1422/hovercard">#1422</a> <a class="user-mention" data-hovercard-type="user" data-hovercard-url="/hovercards?user_id=16285118" data-octo-click="hovercard-link-click" data-octo-dimensions="link_type:self" href="https://urls.greenkeeper.io/golopot">@golopot</a>)</li>
<li>Add support for variables defined as props to <code>prop-types</code> and <code>no-unused-prop-types</code> (<a href="https://urls.greenkeeper.io/yannickcr/eslint-plugin-react/issues/442" data-hovercard-type="issue" data-hovercard-url="/jsx-eslint/eslint-plugin-react/issues/442/hovercard">#442</a> <a href="https://urls.greenkeeper.io/yannickcr/eslint-plugin-react/issues/833" data-hovercard-type="issue" data-hovercard-url="/jsx-eslint/eslint-plugin-react/issues/833/hovercard">#833</a> <a href="https://urls.greenkeeper.io/yannickcr/eslint-plugin-react/issues/1002" data-hovercard-type="issue" data-hovercard-url="/jsx-eslint/eslint-plugin-react/issues/1002/hovercard">#1002</a> <a href="https://urls.greenkeeper.io/yannickcr/eslint-plugin-react/issues/1116" data-hovercard-type="issue" data-hovercard-url="/jsx-eslint/eslint-plugin-react/issues/1116/hovercard">#1116</a> <a href="https://urls.greenkeeper.io/yannickcr/eslint-plugin-react/issues/1257" data-hovercard-type="issue" data-hovercard-url="/jsx-eslint/eslint-plugin-react/issues/1257/hovercard">#1257</a> <a href="https://urls.greenkeeper.io/yannickcr/eslint-plugin-react/issues/1764" data-hovercard-type="issue" data-hovercard-url="/jsx-eslint/eslint-plugin-react/issues/1764/hovercard">#1764</a> <a class="user-mention" data-hovercard-type="user" data-hovercard-url="/hovercards?user_id=16285118" data-octo-click="hovercard-link-click" data-octo-dimensions="link_type:self" href="https://urls.greenkeeper.io/golopot">@golopot</a>)</li>
<li>Add <code>checkFragmentShorthand</code> option to <code>jsx-key</code> (<a href="https://urls.greenkeeper.io/yannickcr/eslint-plugin-react/pull/2316" data-hovercard-type="pull_request" data-hovercard-url="/jsx-eslint/eslint-plugin-react/pull/2316/hovercard">#2316</a> <a class="user-mention" data-hovercard-type="user" data-hovercard-url="/hovercards?user_id=19822240" data-octo-click="hovercard-link-click" data-octo-dimensions="link_type:self" href="https://urls.greenkeeper.io/kaykayehnn">@kaykayehnn</a>)</li>
</ul>
<h3>Fixed</h3>
<ul>
<li>Fix <code>no-did-mount-set-state</code> and <code>no-did-update-set-state</code> to handle cDU and cDM defined as class properties (<a href="https://urls.greenkeeper.io/yannickcr/eslint-plugin-react/issues/1595" data-hovercard-type="issue" data-hovercard-url="/jsx-eslint/eslint-plugin-react/issues/1595/hovercard">#1595</a> <a class="user-mention" data-hovercard-type="user" data-hovercard-url="/hovercards?user_id=1413255" data-octo-click="hovercard-link-click" data-octo-dimensions="link_type:self" href="https://urls.greenkeeper.io/jaaberg">@jaaberg</a>)</li>
<li>Fix <code>sort-prop-types</code> cash when a shape PropType is defined in a variable (<a href="https://urls.greenkeeper.io/yannickcr/eslint-plugin-react/issues/1749" data-hovercard-type="issue" data-hovercard-url="/jsx-eslint/eslint-plugin-react/issues/1749/hovercard">#1749</a> <a class="user-mention" data-hovercard-type="user" data-hovercard-url="/hovercards?user_id=93752" data-octo-click="hovercard-link-click" data-octo-dimensions="link_type:self" href="https://urls.greenkeeper.io/alexzherdev">@alexzherdev</a>)</li>
<li>Fix <code>no-unused-state</code> false positive when using state of non-lifecycle method (<a href="https://urls.greenkeeper.io/yannickcr/eslint-plugin-react/pull/2274" data-hovercard-type="pull_request" data-hovercard-url="/jsx-eslint/eslint-plugin-react/pull/2274/hovercard">#2274</a> <a class="user-mention" data-hovercard-type="user" data-hovercard-url="/hovercards?user_id=16285118" data-octo-click="hovercard-link-click" data-octo-dimensions="link_type:self" href="https://urls.greenkeeper.io/golopot">@golopot</a>)</li>
<li>Fix <code>static-property-placement</code> false positive when accessing static property inside method (<a href="https://urls.greenkeeper.io/yannickcr/eslint-plugin-react/issues/2283" data-hovercard-type="issue" data-hovercard-url="/jsx-eslint/eslint-plugin-react/issues/2283/hovercard">#2283</a> <a class="user-mention" data-hovercard-type="user" data-hovercard-url="/hovercards?user_id=20278756" data-octo-click="hovercard-link-click" data-octo-dimensions="link_type:self" href="https://urls.greenkeeper.io/dmason30">@dmason30</a>)</li>
<li>Fix <code>prop-type</code> detection for annotated props with default value (<a href="https://urls.greenkeeper.io/yannickcr/eslint-plugin-react/issues/2298" data-hovercard-type="issue" data-hovercard-url="/jsx-eslint/eslint-plugin-react/issues/2298/hovercard">#2298</a> <a class="user-mention" data-hovercard-type="user" data-hovercard-url="/hovercards?user_id=13209" data-octo-click="hovercard-link-click" data-octo-dimensions="link_type:self" href="https://urls.greenkeeper.io/yannickcr">@yannickcr</a>)</li>
</ul>
<h3>Changed</h3>
<ul>
<li>Add ESLint 6.0.0 as valid peerDependency (<a class="user-mention" data-hovercard-type="user" data-hovercard-url="/hovercards?user_id=13209" data-octo-click="hovercard-link-click" data-octo-dimensions="link_type:self" href="https://urls.greenkeeper.io/yannickcr">@yannickcr</a>)</li>
<li>Improve <code>no-render-return-value</code> performance (<a href="https://urls.greenkeeper.io/yannickcr/eslint-plugin-react/pull/2259" data-hovercard-type="pull_request" data-hovercard-url="/jsx-eslint/eslint-plugin-react/pull/2259/hovercard">#2259</a> <a class="user-mention" data-hovercard-type="user" data-hovercard-url="/hovercards?user_id=16285118" data-octo-click="hovercard-link-click" data-octo-dimensions="link_type:self" href="https://urls.greenkeeper.io/golopot">@golopot</a>)</li>
<li>Change <code>jsx-sort-props</code> to report errors only on the identifier (<a href="https://urls.greenkeeper.io/yannickcr/eslint-plugin-react/issues/2312" data-hovercard-type="issue" data-hovercard-url="/jsx-eslint/eslint-plugin-react/issues/2312/hovercard">#2312</a> <a class="user-mention" data-hovercard-type="user" data-hovercard-url="/hovercards?user_id=74260" data-octo-click="hovercard-link-click" data-octo-dimensions="link_type:self" href="https://urls.greenkeeper.io/MrHen">@MrHen</a>)</li>
<li>Change to warn only once if react version cannot be detected (<a href="https://urls.greenkeeper.io/yannickcr/eslint-plugin-react/issues/2276" data-hovercard-type="issue" data-hovercard-url="/jsx-eslint/eslint-plugin-react/issues/2276/hovercard">#2276</a> <a class="user-mention" data-hovercard-type="user" data-hovercard-url="/hovercards?user_id=45469" data-octo-click="hovercard-link-click" data-octo-dimensions="link_type:self" href="https://urls.greenkeeper.io/ljharb">@ljharb</a>)</li>
<li>Documentation improvements (<a href="https://urls.greenkeeper.io/yannickcr/eslint-plugin-react/pull/2263" data-hovercard-type="pull_request" data-hovercard-url="/jsx-eslint/eslint-plugin-react/pull/2263/hovercard">#2263</a> <a class="user-mention" data-hovercard-type="user" data-hovercard-url="/hovercards?user_id=15232461" data-octo-click="hovercard-link-click" data-octo-dimensions="link_type:self" href="https://urls.greenkeeper.io/dimitropoulos">@dimitropoulos</a>, <a href="https://urls.greenkeeper.io/yannickcr/eslint-plugin-react/pull/2262" data-hovercard-type="pull_request" data-hovercard-url="/jsx-eslint/eslint-plugin-react/pull/2262/hovercard">#2262</a> <a class="user-mention" data-hovercard-type="user" data-hovercard-url="/hovercards?user_id=473530" data-octo-click="hovercard-link-click" data-octo-dimensions="link_type:self" href="https://urls.greenkeeper.io/ybiquitous">@ybiquitous</a>, <a href="https://urls.greenkeeper.io/yannickcr/eslint-plugin-react/pull/2295" data-hovercard-type="pull_request" data-hovercard-url="/jsx-eslint/eslint-plugin-react/pull/2295/hovercard">#2295</a> <a class="user-mention" data-hovercard-type="user" data-hovercard-url="/hovercards?user_id=1921409" data-octo-click="hovercard-link-click" data-octo-dimensions="link_type:self" href="https://urls.greenkeeper.io/battaglr">@battaglr</a>, <a href="https://urls.greenkeeper.io/yannickcr/eslint-plugin-react/pull/2302" data-hovercard-type="pull_request" data-hovercard-url="/jsx-eslint/eslint-plugin-react/pull/2302/hovercard">#2302</a> <a class="user-mention" data-hovercard-type="user" data-hovercard-url="/hovercards?user_id=5185660" data-octo-click="hovercard-link-click" data-octo-dimensions="link_type:self" href="https://urls.greenkeeper.io/Jason-Cooke">@Jason-Cooke</a>, <a href="https://urls.greenkeeper.io/yannickcr/eslint-plugin-react/pull/2303" data-hovercard-type="pull_request" data-hovercard-url="/jsx-eslint/eslint-plugin-react/pull/2303/hovercard">#2303</a> <a class="user-mention" data-hovercard-type="user" data-hovercard-url="/hovercards?user_id=16285118" data-octo-click="hovercard-link-click" data-octo-dimensions="link_type:self" href="https://urls.greenkeeper.io/golopot">@golopot</a>)</li>
<li>Code refactoring (<a href="https://urls.greenkeeper.io/yannickcr/eslint-plugin-react/pull/2265" data-hovercard-type="pull_request" data-hovercard-url="/jsx-eslint/eslint-plugin-react/pull/2265/hovercard">#2265</a> <a href="https://urls.greenkeeper.io/yannickcr/eslint-plugin-react/pull/2267" data-hovercard-type="pull_request" data-hovercard-url="/jsx-eslint/eslint-plugin-react/pull/2267/hovercard">#2267</a> <a href="https://urls.greenkeeper.io/yannickcr/eslint-plugin-react/pull/2286" data-hovercard-type="pull_request" data-hovercard-url="/jsx-eslint/eslint-plugin-react/pull/2286/hovercard">#2286</a> <a href="https://urls.greenkeeper.io/yannickcr/eslint-plugin-react/pull/2294" data-hovercard-type="pull_request" data-hovercard-url="/jsx-eslint/eslint-plugin-react/pull/2294/hovercard">#2294</a> <a class="user-mention" data-hovercard-type="user" data-hovercard-url="/hovercards?user_id=16285118" data-octo-click="hovercard-link-click" data-octo-dimensions="link_type:self" href="https://urls.greenkeeper.io/golopot">@golopot</a>, <a class="user-mention" data-hovercard-type="user" data-hovercard-url="/hovercards?user_id=45469" data-octo-click="hovercard-link-click" data-octo-dimensions="link_type:self" href="https://urls.greenkeeper.io/ljharb">@ljharb</a>)</li>
<li>Tests improvements (<a href="https://urls.greenkeeper.io/yannickcr/eslint-plugin-react/pull/2304" data-hovercard-type="pull_request" data-hovercard-url="/jsx-eslint/eslint-plugin-react/pull/2304/hovercard">#2304</a> <a href="https://urls.greenkeeper.io/yannickcr/eslint-plugin-react/issues/1047" data-hovercard-type="issue" data-hovercard-url="/jsx-eslint/eslint-plugin-react/issues/1047/hovercard">#1047</a> <a class="user-mention" data-hovercard-type="user" data-hovercard-url="/hovercards?user_id=16285118" data-octo-click="hovercard-link-click" data-octo-dimensions="link_type:self" href="https://urls.greenkeeper.io/golopot">@golopot</a>, <a class="user-mention" data-hovercard-type="user" data-hovercard-url="/hovercards?user_id=13209" data-octo-click="hovercard-link-click" data-octo-dimensions="link_type:self" href="https://urls.greenkeeper.io/yannickcr">@yannickcr</a>)</li>
</ul>
</details>

<details>
<summary>Commits</summary>
<p>The new version differs by 68 commits.</p>
<ul>
<li><a href="https://urls.greenkeeper.io/yannickcr/eslint-plugin-react/commit/dfaa92f34ac39fa8c320068501ec86fe0b9c8122"><code>dfaa92f</code></a> <code>Update CHANGELOG and bump version</code></li>
<li><a href="https://urls.greenkeeper.io/yannickcr/eslint-plugin-react/commit/c52b61b0d371b956b6c6fef3240839f4cea2ffa7"><code>c52b61b</code></a> <code>Merge pull request #2316 from kaykayehnn/jsx-key-fragments</code></li>
<li><a href="https://urls.greenkeeper.io/yannickcr/eslint-plugin-react/commit/8db631b400b242266be38202926d86c244e8d116"><code>8db631b</code></a> <code>[Fix] Fix detection of annotated props with default value</code></li>
<li><a href="https://urls.greenkeeper.io/yannickcr/eslint-plugin-react/commit/bbebefd2292294e892f743993f2cc778e3e36a85"><code>bbebefd</code></a> <code>[Tests] Remove AppVeyor</code></li>
<li><a href="https://urls.greenkeeper.io/yannickcr/eslint-plugin-react/commit/0d49f5abd1d5423a7a305d26cad9a7f5b046bc89"><code>0d49f5a</code></a> <code>[New] Add ESLint ^6.0.0 as valid peerDependency</code></li>
<li><a href="https://urls.greenkeeper.io/yannickcr/eslint-plugin-react/commit/0364ed210b95f1a14bda0180a3d194cf9e6b7176"><code>0364ed2</code></a> <code>Fix formatting issues</code></li>
<li><a href="https://urls.greenkeeper.io/yannickcr/eslint-plugin-react/commit/7c1abedb575bd97404933ec85144697a30778443"><code>7c1abed</code></a> <code>Add checkFragmentShorthand option</code></li>
<li><a href="https://urls.greenkeeper.io/yannickcr/eslint-plugin-react/commit/ed04c2fd48a1c1ad21daade6225229e281446c6a"><code>ed04c2f</code></a> <code>Fix tests in eslint &lt; 5</code></li>
<li><a href="https://urls.greenkeeper.io/yannickcr/eslint-plugin-react/commit/0d1aaf8b569e6d22bba90467fe46ee7062e5e5ba"><code>0d1aaf8</code></a> <code>Handle fragments in jsx-key</code></li>
<li><a href="https://urls.greenkeeper.io/yannickcr/eslint-plugin-react/commit/7d449a9e4a1330ab950df4b55348d6eff5d025af"><code>7d449a9</code></a> <code>[New] <code>jsx-sort-props</code>: Change reported range to only the identifier</code></li>
<li><a href="https://urls.greenkeeper.io/yannickcr/eslint-plugin-react/commit/1e102f0d3281c24cd1b99c27bdbf63c14d6bcde2"><code>1e102f0</code></a> <code>Change reported range to only the identifier</code></li>
<li><a href="https://urls.greenkeeper.io/yannickcr/eslint-plugin-react/commit/e6b4c33a1db4cc94c3e9223b09fb92b1dbddc00d"><code>e6b4c33</code></a> <code>Merge pull request #2301 from golopot/fix-cached-props-2</code></li>
<li><a href="https://urls.greenkeeper.io/yannickcr/eslint-plugin-react/commit/9a63e19460e7ddbf988f328cf9f9730f4f156e0d"><code>9a63e19</code></a> <code>Immediately destructure out propVariables rather than using it as a namespace</code></li>
<li><a href="https://urls.greenkeeper.io/yannickcr/eslint-plugin-react/commit/3a1a0d1d159efd6bb3c597671e4ec8afaee22018"><code>3a1a0d1</code></a> <code>Apply suggestion: replace mutation with Object.assign</code></li>
<li><a href="https://urls.greenkeeper.io/yannickcr/eslint-plugin-react/commit/89b8143ac18f4d3f0d45717436ad2aa344e2d494"><code>89b8143</code></a> <code>Apply suggestion: replace concat([a]) with concat(a)</code></li>
</ul>
<p>There are 68 commits in total.</p>
<p>See the <a href="https://urls.greenkeeper.io/yannickcr/eslint-plugin-react/compare/f39829ffb3134fb1298c7e96a4349eb835f15877...dfaa92f34ac39fa8c320068501ec86fe0b9c8122">full diff</a></p>
</details>

<details>
  <summary>FAQ and help</summary>

  There is a collection of [frequently asked questions](https://greenkeeper.io/faq.html). If those don’t help, you can always [ask the humans behind Greenkeeper](https://github.com/greenkeeperio/greenkeeper/issues/new).
</details>

---


Your [Greenkeeper](https://greenkeeper.io) bot 🌴
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.

None yet

3 participants