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

Re-exported React.memo in packages/element/src/react.js #15385

Merged
merged 3 commits into from
May 3, 2019
Merged

Re-exported React.memo in packages/element/src/react.js #15385

merged 3 commits into from
May 3, 2019

Conversation

nicolad
Copy link
Member

@nicolad nicolad commented May 2, 2019

Description

Resolves: #15242

Types of changes

This PR is re-exporting React.js core memo method.
Also refactored concatChildren internal memo to avoid conflicts with global one.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@nicolad nicolad changed the title Refactored concatChildren internal memo. Re-exported React.memo May 2, 2019
@nicolad nicolad changed the title Re-exported React.memo Re-exported React.memo in packages/element/src/react.js May 2, 2019
@@ -179,18 +185,18 @@ export { Suspense };
* @return {Array} The concatenated value.
*/
export function concatChildren( ...childrenArguments ) {
return childrenArguments.reduce( ( memo, children, i ) => {
return childrenArguments.reduce( ( memoized, children, i ) => {
Copy link
Member

@aduth aduth May 2, 2019

Choose a reason for hiding this comment

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

memo was a poor choice of name to begin with (myself to blame), and was largely inherited from old Underscore.js documentation. There's a bit of inconsistency in how this argument is named elsewhere in the codebase, but "memoized" is not one of them, nor reflective of what the value is.

I might suggest result instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed, result is a better name for an accumulator. Thx.

Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

Looks good 👍

I'm assuming the prior build failure was an intermittent failure. I've restarted it.

@nicolad
Copy link
Member Author

nicolad commented May 3, 2019

Looks good

I'm assuming the prior build failure was an intermittent failure. I've restarted it.

All good now, but I'm not authorized to merge this :(
p.s.: is there a required number of merged PRs in order to be added to org or to have the possibility to merge own PRs ? my merged PRs: https://github.com/WordPress/gutenberg/pulls?q=is%3Apr+is%3Aclosed+author%3Anicolad

@aduth
Copy link
Member

aduth commented May 3, 2019

All good now, but I'm not authorized to merge this :(

I'll merge it 👍

p.s.: is there a required number of merged PRs in order to be added to org or to have the possibility to merge own PRs ? my merged PRs: https://github.com/WordPress/gutenberg/pulls?q=is%3Apr+is%3Aclosed+author%3Anicolad

It's a good question. I'm not the one to dole out permissions, but I think it may be good to raise this in next week's editor chat, both on the general point of prerequisites and on the specific point of having yourself granted access. I could raise it on your behalf if you're unable to attend.

@aduth aduth merged commit 3335c0b into WordPress:master May 3, 2019
@nicolad
Copy link
Member Author

nicolad commented May 3, 2019

I could raise it on your behalf if you're unable to attend.

That would be amazing since I might have some job-related meetings at the same time.
Thank you, have a nice weekend.

@@ -106,6 +107,11 @@ export { Fragment };
*/
export { isValidElement };

/**
* @see https://reactjs.org/docs/react-api.html#reactmemo
Copy link
Member

Choose a reason for hiding this comment

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

It should be included in CHANGELOG that we now expose memo as well.

React.memo is a higher order component. It’s similar to React.PureComponent but for function components instead of classes.

Are we going to promote it over pure HOC we offer in @wordpress/compose package? Should we refactor pure to use memo internally? /cc @youknowriad

Copy link
Contributor

Choose a reason for hiding this comment

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

I think there are differences where pure applies to any component and memo only to function components?

@youknowriad youknowriad added this to the 5.7 (Gutenberg) milestone May 10, 2019
@nicolad nicolad deleted the re-export-react-memo branch September 29, 2019 14:50
@nicolad nicolad self-assigned this Sep 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

wp.element should re-export React.memo
4 participants