Soft deprecate custom 'pure' HoC in favor of 'React.memo' #57173
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Mamaduka
added
[Type] Code Quality
|
Size Change: +7 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
ellatrix
reviewed
Dec 18, 2023
Do you mean to update our custom HoC to use Yes, I was thinking the same. There are a few more usages of |
|
Sure, why not? Don't see the point of just updating one instance? |
|
Makes sense. I'll make the remaining changes tomorrow morning. |
Mamaduka
changed the title
Mamaduka
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Mamaduka
force-pushed
the
try/block-support-replace-pure-with-memo
branch
from
dd0b0ac
to
df2f93c
Compare
Mamaduka
changed the title
|
cc @WordPress/native-mobile |
Hey @Mamaduka, I can take a look at the changes and see how they impact the native mobile version. I see that both mobile E2E and unit tests are failing, so it's likely that we need to tweak some parts of the native files. I'll share more information once I investigate it. Thanks 🙇 ! |
tyxla
reviewed
Dec 20, 2023
| @@ -141,6 +141,8 @@ _Related_ | |||
|
|
|||
| ### pure | |||
|
|
|||
| > **Deprecated** Use `memo` or `PureComponent` instead. | |||
There was a problem hiding this comment.
Should we make it clear that memo is from @wordpress/element or react?
There was a problem hiding this comment.
I can't find the issue, but I remember there was a talk of allowing imports from react. So folks can decide that for themself.
| @@ -141,6 +141,8 @@ _Related_ | |||
|
|
|||
| ### pure | |||
|
|
|||
| > **Deprecated** Use `memo` or `PureComponent` instead. | |||
There was a problem hiding this comment.
Also, we don't expose PureComponent at all in @wordpress/element, do we? So it can be confusing to mention it in the first place.
There was a problem hiding this comment.
The only reason the PureComponent was omitted was because of this HoC. See #33674 (comment).
| @@ -17,10 +18,17 @@ import { createHigherOrderComponent } from '../../utils/create-higher-order-comp | |||
| /** | |||
| * Given a component returns the enhanced component augmented with a component | |||
| * only re-rendering when its props/state change | |||
| * | |||
| * @deprecated Use `memo` or `PureComponent` instead. | |||
| */ | ||
| const pure = createHigherOrderComponent( function < Props extends {} >( | ||
| WrappedComponent: ComponentType< Props > | ||
| ): ComponentType< Props > { | ||
| deprecated( 'wp.compose.pure', { | ||
| since: '6.5', | ||
| alternative: 'Use `memo` or `PureComponent` instead', |
fluiddot
reviewed
Dec 21, 2023
Comment on lines
56
to
57
| pure( ( ownProps ) => { | ||
| memo( ( ownProps ) => { |
There was a problem hiding this comment.
@Mamaduka After testing the mobile native version, I noticed a nuance here. When using memo, if I try to get the reference via the ref prop in the resulting component, I get the following warning:
Warning: Function components cannot be given refs. Attempts to access this ref will fail. Did you mean to use React.forwardRef()?
I explored further and realized that the difference between using pure and memo is that each returns different types:
purereturns class component.memoreturns function component.
For the latter, I can't use forwardRef anymore for forwarding the ref after using withSelect. I wonder if we should implement a mechanism for this. WDYT?
There was a problem hiding this comment.
Thanks for looking into this, @fluiddot!
Can you link me to one of these components? I would like to experiment a bit with ideas.
There was a problem hiding this comment.
Sure, the component that was failing is this one which is used here. Note that both are only used in the mobile native version, so you'd need to run the demo app to test it.
In any case, since the issue is not only affecting the mobile native version, I'd recommend tweaking the code with a test component for testing. I took the liberty to generate the following patch file as a demonstration of the issue:
diff --git forkSrcPrefix/packages/edit-post/src/components/visual-editor/index.js forkDstPrefix/packages/edit-post/src/components/visual-editor/index.js
index fd9b4a6ff8bb6cc7d4ba5145bc7928259f18a33a..82cd77a5b039d5ce50d6f5503d39df2f7d4ba63d 100644
--- forkSrcPrefix/packages/edit-post/src/components/visual-editor/index.js
+++ forkDstPrefix/packages/edit-post/src/components/visual-editor/index.js
@@ -10,8 +10,8 @@ import {
store as editorStore,
privateApis as editorPrivateApis,
} from '@wordpress/editor';
-import { useMemo } from '@wordpress/element';
-import { useSelect } from '@wordpress/data';
+import { useMemo, Component } from '@wordpress/element';
+import { useSelect, withSelect } from '@wordpress/data';
import { store as blocksStore } from '@wordpress/blocks';
/**
@@ -24,6 +24,13 @@ const { EditorCanvas } = unlock( editorPrivateApis );
const isGutenbergPlugin = process.env.IS_GUTENBERG_PLUGIN ? true : false;
+class ClassComponent extends Component {
+ render() {
+ return null;
+ }
+}
+const ClassComponentWithSelect = withSelect( () => ( {} ) )( ClassComponent );
+
export default function VisualEditor( { styles } ) {
const {
isWelcomeGuideVisible,
@@ -83,6 +90,10 @@ export default function VisualEditor( { styles } ) {
'has-inline-canvas': ! isToBeIframed,
} ) }
>
+ { /* This works */ }
+ <ClassComponent ref={ () => {} } />
+ { /* This produces a warning. */ }
+ <ClassComponentWithSelect ref={ () => {} } />
<EditorCanvas
disableIframe={ ! isToBeIframed }
styles={ styles }
There was a problem hiding this comment.
We should deprecate withSelect too :P
|
Flaky tests detected in 0939b95. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7299927486
|
|
I am currently considering soft-deprecating This way we can maintain backward compatibility for hooks like What do you think? |
ellatrix
reviewed
Jan 19, 2024
| @@ -55,7 +55,7 @@ function ScaledBlockPreview( { | |||
| }, [ styles, additionalStyles ] ); | |||
|
|
|||
| // Initialize on render instead of module top level, to avoid circular dependency issues. | |||
| MemoizedBlockList = MemoizedBlockList || pure( BlockList ); | |||
| MemoizedBlockList = MemoizedBlockList || memo( BlockList ); | |||
There was a problem hiding this comment.
Hm, this feels super weird! Does this even work? :D
There was a problem hiding this comment.
I think so 😅 there is even a Babel plugin that applies similar optimizations - https://babeljs.io/docs/babel-plugin-transform-react-constant-elements.html
There was a problem hiding this comment.
This was marked as an error by the new eslint-plugin-react-compiler plugin - https://github.com/WordPress/gutenberg/actions/runs/9158471368/job/25176908344?pr=61788#step:5:86.
ellatrix
reviewed
Jan 19, 2024
| @@ -74,6 +84,9 @@ describe( 'pure', () => { | |||
| ); | |||
|
|
|||
| const { rerender } = render( <MyComp /> ); | |||
| expect( console ).toHaveWarnedWith( | |||
| 'wp.compose.pure is deprecated since version 6.5. Please use Use `memo` or `PureComponent` instead instead.' | |||
There was a problem hiding this comment.
ellatrix
approved these changes
Jan 19, 2024
Mamaduka
force-pushed
the
try/block-support-replace-pure-with-memo
branch
from
0939b95
to
044154f
Compare
Mamaduka
changed the title
tyxla
approved these changes
Jan 19, 2024
There was a problem hiding this comment.
Looks good to me, thanks! 👍
Left a couple of minor comments, nothing blocking.
I also wanted to ask if we're intentionally leaving the pure() usage for:
withSelect: https://github.com/WordPress/gutenberg/blob/try/block-support-replace-pure-with-memo/packages/data/src/components/with-select/index.js#L56withViewportMatch: https://github.com/WordPress/gutenberg/blob/try/block-support-replace-pure-with-memo/packages/viewport/src/with-viewport-match.js#L51
Yes, to avoid making this a breaking change. See #57173 (comment) and #57173 (comment). |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Requires: #57193.
PR replaces
pureusage withReact.memowhere possible and soft deprecates the custom HoC.Why?
I think the core custom HoC pre-dates
React.memoand maybe evenPureComponent. Using APIs provided by React has the benefit of inheriting all optimizations that the library might ship in the future.Testing Instructions