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

Annotations & Performance: remove use of Memize #14664

Merged
merged 1 commit into from
Apr 3, 2019

Conversation

ellatrix
Copy link
Member

@ellatrix ellatrix commented Mar 27, 2019

Description

Split out from #14641.

Some functions were memoized in #12161, because new array and objects are returned as props. We can avoid this by prefixing the prop keys (as is already done in one case).

How has this been tested?

Screenshots

Types of changes

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.

@ellatrix ellatrix added [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable [Feature] Annotations Adding annotation functionality labels Mar 27, 2019
@ellatrix ellatrix force-pushed the try/annotations-remove-memize branch 2 times, most recently from f4c6ffb to 7399888 Compare March 28, 2019 08:12
@@ -25,6 +25,7 @@
"@wordpress/compose": "file:../compose",
"@wordpress/data": "file:../data",
"@wordpress/escape-html": "file:../escape-html",
"@wordpress/hooks": "file:../hooks",
Copy link
Member Author

Choose a reason for hiding this comment

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

I saw this dependency was missing.

@ellatrix ellatrix added the [Package] Rich text /packages/rich-text label Mar 28, 2019
@ellatrix ellatrix added this to the 5.4 (Gutenberg) milestone Mar 28, 2019
@ellatrix ellatrix added [Type] Code Quality Issues or PRs that relate to code quality [Type] Performance Related to performance efforts labels Mar 28, 2019
@ellatrix ellatrix mentioned this pull request Mar 28, 2019
5 tasks
Copy link
Member

@atimmer atimmer left a comment

Choose a reason for hiding this comment

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

I've done a quick code review and this makes sense conceptually.

Also, I've tested with Yoast SEO and everything still works quick. 🎉 Great work!

packages/block-editor/src/components/rich-text/index.js Outdated Show resolved Hide resolved
@ellatrix ellatrix force-pushed the try/annotations-remove-memize branch from 382baa8 to e457a8b Compare April 3, 2019 15:33
@ellatrix
Copy link
Member Author

ellatrix commented Apr 3, 2019

Thanks @atimmer for the review!

@ellatrix ellatrix merged commit 1443f45 into master Apr 3, 2019
@ellatrix ellatrix deleted the try/annotations-remove-memize branch April 3, 2019 15:56
@@ -27,7 +27,6 @@
"@wordpress/i18n": "file:../i18n",
"@wordpress/rich-text": "file:../rich-text",
"lodash": "^4.17.11",
"memize": "^1.0.5",
Copy link
Member

Choose a reason for hiding this comment

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

package-lock.json should have been updated as a result of this change. There's local revisions after running npm install in master.

Aside: Travis should be catching this, and it's unclear why it's not.

Copy link
Member

Choose a reason for hiding this comment

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

See #14808

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Annotations Adding annotation functionality [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable [Package] Rich text /packages/rich-text [Type] Code Quality Issues or PRs that relate to code quality [Type] Performance Related to performance efforts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants