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.

Sign up for GitHub

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

Jump to bottom

Packages: Extract entities package #7977

Merged
merged 4 commits into from
Jul 16, 2018
Merged

Packages: Extract entities package #7977

merged 4 commits into from
Jul 16, 2018

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented Jul 16, 2018 •
edited
Loading

Description

Part of #3955.

This PR extracts new @wordpress/entities package which contains one utility method decodeEntities.

I also added 2 unrelated changes:

  • missing .npmrc files to a few packages
  • updated deprecated usage of @wordpress/keycodes

How has this been tested?

npm test and make sure there are no regressions.

wp.utils.decodeEntities should work as before but include warning informing about planned deprecation, e.g.

wp.utils.decodeEntities( '<x>&amp;</x>' )

@gziolo gziolo added [Type] Breaking Change For PRs that introduce a change that will break existing functionality npm Packages Related to npm packages labels Jul 16, 2018
@gziolo gziolo added this to the 3.3 milestone Jul 16, 2018
@gziolo gziolo self-assigned this Jul 16, 2018
@gziolo gziolo requested review from youknowriad and a team July 16, 2018 11:15
tofumatt
tofumatt reviewed Jul 16, 2018
View reviewed changes
Copy link
Member

@tofumatt tofumatt 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 to me, but curious about the removal of wp-blob from the wp_register_script call.

lib/client-assets.php
@@ -199,7 +206,7 @@ function gutenberg_register_scripts_and_styles() {
wp_register_script(
'wp-utils',
gutenberg_url( 'build/utils/index.js' ),
array( 'lodash', 'wp-blob', 'wp-deprecated', 'wp-api-request', 'wp-i18n', 'wp-keycodes' ),
Copy link
Member

Choose a reason for hiding this comment

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

Why is blob removed here? Is that intentional/related to this change?

Copy link
Member Author

Choose a reason for hiding this comment

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

It should be removed in one of the previous PRs, it's no longer used inside.

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 forgot to mention. Sorry about it.

Copy link
Member

Choose a reason for hiding this comment

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

Ah okay, just wasn't sure it was related. 👍

packages/entities/README.md Outdated
@@ -0,0 +1,13 @@
# @wordpress/entities

Entities utils for WordPress.
Copy link
Member

Choose a reason for hiding this comment

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

Can we at least say "keyboard entities" or something? 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

Should we call it html-entities?

Copy link
Member Author

Choose a reason for hiding this comment

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

Those are HTML entities, btw :)

Copy link
Member

Choose a reason for hiding this comment

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

Oh geez, that makes more sense. I think I read the TAB import change at the top of the PR diff and my brain got confused. Time for more ☕️

"HTML Entities" would be great.

tofumatt
tofumatt approved these changes Jul 16, 2018
View reviewed changes
Copy link
Member

@tofumatt tofumatt 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 after the blob explanation. I'd tweak the README for the new module, but otherwise 👍

@gziolo
Copy link
Member Author

gziolo commented Jul 16, 2018

It looks like we introduced regression in e2e tests earlier today when merging changes that introduce compose package.

@gziolo
Copy link
Member Author

gziolo commented Jul 16, 2018

There are a few possibilities in regards to the package name:

  • entities
  • format
  • html-entities
  • string

@aduth, @tofumatt, @youknowriad any thoughts?

@gziolo gziolo mentioned this pull request Jul 16, 2018
Merged
@tofumatt
Copy link
Member

html-entities is clearest to me and I think works. Let's do that.

@aduth
Copy link
Member

aduth commented Jul 16, 2018

Related: #7824 WordPress/packages#12 (specifically WordPress/packages#12 (comment) )

Was a point that it could be included in the proposed sanitize package, though HTML entity encoding isn't exclusively related to sanitization, so maybe not the most accurate grouping.

@gziolo
Copy link
Member Author

gziolo commented Jul 16, 2018

Let's go with html-entities we can always merge a few packages into a larger one.

@aduth
Copy link
Member

aduth commented Jul 16, 2018

Prior art: The he package is probably one of the most popular in this space, and is an acronym of "html entities", so html-entities is a safe bet.

And in case anyone is tempted to lodge the NIH complaint: Take a look at the built size of he or similar alternatives and you'll understand why it's been avoided for use in a web project 😉

@gziolo
Copy link
Member Author

gziolo commented Jul 16, 2018

http://php.net/manual/en/function.html-entity-decode.php - this will match with PHP version of similar functionality.

@gziolo
Copy link
Member Author

gziolo commented Jul 16, 2018

And in case anyone is tempted to lodge the NIH complaint: Take a look at the built size of he or similar alternatives and you'll understand why it's been avoided for use in a web project 😉

82.8 KB :) Good to know about that in case we would need something working in Node or RN env.

@aduth
Copy link
Member

aduth commented Jul 16, 2018 •
edited
Loading

82.8 KB :) Good to know about that in case we would need something working in Node or RN env.

Yes, in fact it was implemented this way in Calypso for Node vs. browser:

https://github.com/Automattic/wp-calypso/blob/master/client/lib/formatting/decode-entities/browser.js
https://github.com/Automattic/wp-calypso/blob/master/client/lib/formatting/decode-entities/node.js

I'm not entirely sure it's great to have the separate implementations. From what I recall, the browser-based implementation (at least without shortcutting/caching) is quite non-performant as well (Automattic/wp-calypso#9725).

@gziolo gziolo merged commit 6299acb into master Jul 16, 2018
@gziolo gziolo deleted the add/entities-package branch July 16, 2018 14:28
@gziolo gziolo added the [Package] HTML entities /packages/html-entities label Jul 16, 2018
@aduth aduth mentioned this pull request Mar 20, 2019
Closed
@gwwar gwwar mentioned this pull request Dec 18, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tofumatt tofumatt tofumatt approved these changes

@youknowriad youknowriad Awaiting requested review from youknowriad

Assignees

@gziolo gziolo

Labels
npm Packages Related to npm packages [Package] HTML entities /packages/html-entities [Type] Breaking Change For PRs that introduce a change that will break existing functionality
Projects
None yet
Milestone
3.3
Development

Successfully merging this pull request may close these issues.
3 participants
@gziolo @tofumatt @aduth