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

[Docs] no-unstable-nested-components: Warn about memoized, nested components #3444

Merged
merged 1 commit into from
Oct 7, 2022

Conversation

eps1lon
Copy link
Contributor

@eps1lon eps1lon commented Sep 29, 2022

The issue this rule catches is a very common beginner issue that requires understanding of the reconciler. Ideally this issue wouldn't exist but until then let's make sure you start out with a linter that catches these issues.

I adjusted the docs to no longer recommend memoizing nested component and document migration strategies.

/cc @gaearon

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Adding anything to a shared config is a semver-major change, so it's unlikely to land any time soon. I'd love to land the docs updates separately tho!

@ljharb ljharb added the semver-major Breaking change. label Sep 29, 2022
@eps1lon
Copy link
Contributor Author

eps1lon commented Sep 29, 2022

Adding anything to a shared config is a semver-major change, so it's unlikely to land any time soon. I'd love to land the docs updates separately tho!

Gotcha. Let's discuss the docs changes then and I'll file the semver-major separately.

@gaearon
Copy link
Collaborator

gaearon commented Sep 29, 2022

Should we just add this logic to rules of Hooks?

@eps1lon eps1lon changed the title no-unstable-nested-components: Add to recommended rules docs(no-unstable-nested-components): Warn about memoized, nested components Sep 29, 2022
@eps1lon eps1lon marked this pull request as ready for review September 29, 2022 16:51
@codecov
Copy link

codecov bot commented Sep 29, 2022

Codecov Report

Merging #3444 (2575bba) into master (b52e0ca) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 2575bba differs from pull request most recent head f204f11. Consider uploading reports for the commit f204f11 to get more accurate results

@@           Coverage Diff           @@
##           master    #3444   +/-   ##
=======================================
  Coverage   97.62%   97.62%           
=======================================
  Files         123      123           
  Lines        8959     8962    +3     
  Branches     3272     3275    +3     
=======================================
+ Hits         8746     8749    +3     
  Misses        213      213           
Impacted Files Coverage Δ
lib/rules/no-arrow-function-lifecycle.js 98.43% <100.00%> (+0.07%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@ljharb
Copy link
Member

ljharb commented Sep 29, 2022

@gaearon if it's hook-related then it definitely makes sense.


React reconciliation performs element type comparison with [reference equality](https://github.com/facebook/react/blob/v16.13.1/packages/react-reconciler/src/ReactChildFiber.js#L407). The reference to the same element changes on each re-render when defining components inside the render block. This leads to complete recreation of the current node and all its children. As a result the virtual DOM has to do extra unnecessary work and [possible bugs are introduced](https://codepen.io/ariperkkio/pen/vYLodLB).
React reconciliation performs element type comparison with [reference equality](https://reactjs.org/docs/reconciliation.html#elements-of-different-types). The reference to the same element changes on each re-render when defining components inside the render block. This leads to complete recreation of the current node and all its children. As a result the virtual DOM has to do extra unnecessary work and [possible bugs are introduced](https://codepen.io/ariperkkio/pen/vYLodLB).
Copy link
Contributor

Choose a reason for hiding this comment

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

[possible bugs are introduced](https://codepen.io/ariperkkio/pen/vYLodLB)

I'll update this demo not to include useCallback example at some point.

@AriPerkkio
Copy link
Contributor

If I remember right this is the line that allows memoized nested components:

// Do not mark components declared inside hooks (or falsy '() => null' clean-up methods)
|| isReturnStatementOfHook(node, context)

This was clearly a mistake in the rule's initial design. useCallback etc. don't guarantee the component is actually created just once. And moving the component out of the render block should always be the first step to recommend.

@ljharb ljharb force-pushed the feat/no-unstable-rec branch 2 times, most recently from c3e74d2 to 78ad0f0 Compare October 7, 2022 17:06
@ljharb ljharb added documentation and removed semver-major Breaking change. labels Oct 7, 2022
@ljharb ljharb changed the title docs(no-unstable-nested-components): Warn about memoized, nested components [Docs] no-unstable-nested-components: Warn about memoized, nested components Oct 7, 2022
@ljharb ljharb merged commit 78ad0f0 into jsx-eslint:master Oct 7, 2022
@eps1lon eps1lon deleted the feat/no-unstable-rec branch October 8, 2022 06:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants