Framework: Substitute crypto polyfill dependants with smaller stand-ins #17356

aduth · 2017-08-18T22:03:56Z

A few of our dependencies, namely component-uid and js-sha1, themselves have dependencies on crypto:

https://github.com/emn178/js-sha1/blob/c724643/src/sha1.js#L52-L53
https://github.com/component/uid/blob/2d912b8/index.js#L45 (It appears they try to avoid the polyfill, but it is included in a Webpack build)

crypto is a Node built-in module which is polyfilled in the browser by crypto-browserify. This polyfill is quite substantial in size.

The changes here seek to find suitable replacements for each: ~~uuid~~ (Lodash's uniqueId) and hash.js respectively, which are very reasonable in size.

Aside: I first suspected uuid to also require crypto, but it turns out they bundle a browser-specific variant without the dependency.

The difference is pretty noticeable: 472kb smaller minified (112kb gzipped) in the main build.js.

Before:

wp-calypso (master) $ wc -c public/build.b1e9a12a27f881b1406e.js
 7190661 public/build.b1e9a12a27f881b1406e.js
wp-calypso (master) $ gzip-size public/build.b1e9a12a27f881b1406e.js --raw
1275631

After:

wp-calypso (master) $ wc -c public/build.10d51aa5c5ffef307acf.js
 6718680 public/build.10d51aa5c5ffef307acf.js
wp-calypso (master) $ gzip-size public/build.10d51aa5c5ffef307acf.js --raw
1163240

Testing instructions:

Verify against regressions for the two modules affected:

Sync handler (used for caching response data on the posts list)
Popover (tooltips, ellipsis menu, etc)

Ensure unit tests pass:

npm test

matticbot · 2017-08-18T22:03:57Z

Test live: https://calypso.live/?branch=remove/crypto-dependency

gwwar · 2017-08-19T00:01:49Z

I'm not sure if it's related but devdocs/blocks isn't loading for me on this branch.

aidvu · 2017-08-19T09:08:06Z

@gwwar It was fixed: 925af92.

retrofox · 2017-08-19T12:01:36Z

client/components/popover/index.jsx

@@ -361,7 +361,7 @@ class Popover extends Component {
 	}

 	setPopoverId( id ) {
-		this.id = id || `pop__${ uid( 16 ) }`;
+		this.id = id || `pop__${ uuid() }`;


This line is almost unneeded. If we think that it's better to get rid of it let's do it. I could do it in a next PR as well.

A static incrementing number could work just as well, yeah. (Or nothing at all)

Yeah, lodash/uniqueId seems better (and simpler) for this case. It actually guarantees that it will be unique, while uuid can't.

retrofox · 2017-08-19T12:03:28Z

client/lib/wp/sync-handler/utils.js

@@ -1,11 +1,10 @@
 /**
 * External dependencies
 */
-import sha1 from 'js-sha1';
+import sha1 from 'hash.js/lib/hash/sha/1';


in fact, we could get rid of sync-handler as well. In this way, we could clean this module completely.

in fact, we could get rid of sync-handler as well. In this way, we could clean this module completely.

Yeah, I figured sync handler is on its way out. Most of the pieces are already there with the newer custom post type screen (i.e. caching post responses for future use). I think the remaining issue is whether we need to preserve existing UIs (especially with newer features like re-Publicize) and how we extend CPT components to support those cases. Regardless, it's a bigger issue than one I want to address here, and I think it'll be good to keep the sync handler around until a full replacement can be made.

matticbot · 2017-08-21T11:50:44Z

@aduth This PR needs a rebase

blowery · 2017-08-22T13:27:07Z

The difference is pretty noticeable: 472kb smaller minified (112kb gzipped)

😲

matticbot · 2017-08-29T11:23:18Z

@aduth This PR needs a rebase

alisterscott · 2017-08-30T01:45:51Z

I did some testing of this and couldn't find any regressions to areas mentioned 👍

aduth · 2017-08-30T16:20:12Z

Rebased and replaced the Popover's uuid with Lodash uniqueId, regenerated shrinkwrap once more, did a final round of testing, and will merge shortly.

gwwar · 2017-08-30T17:16:08Z

@aduth @tyxla heads up on overlapping changes in #17560

gwwar

Thanks @aduth this tests well for me

aduth · 2017-08-30T17:44:35Z

Thanks for catching the overlap @gwwar

tyxla · 2017-08-30T19:38:51Z

Thanks @aduth @gwwar!

Obsolete as of #17356.

aduth added Framework [Type] Performance [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Aug 18, 2017

aduth requested review from blowery, retrofox and gwwar August 18, 2017 22:03

matticbot added the [Size] XL Probably needs to be broken down into multiple smaller issues label Aug 18, 2017

aduth requested a review from aidvu August 18, 2017 22:04

retrofox reviewed Aug 19, 2017

View reviewed changes

matticbot added the [Status] Needs Rebase label Aug 21, 2017

aduth force-pushed the remove/crypto-dependency branch from 2569696 to 7722dd2 Compare August 21, 2017 15:01

aduth removed the [Status] Needs Rebase label Aug 21, 2017

matticbot added the [Status] Needs Rebase label Aug 29, 2017

aduth force-pushed the remove/crypto-dependency branch from 7722dd2 to 1558328 Compare August 29, 2017 12:40

aduth removed the [Status] Needs Rebase label Aug 29, 2017

Framework: Substitute crypto polyfill with smaller stand-ins

5dde395

aduth force-pushed the remove/crypto-dependency branch from 1558328 to 5dde395 Compare August 30, 2017 16:19

aduth added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Aug 30, 2017

gwwar approved these changes Aug 30, 2017

View reviewed changes

aduth merged commit c4f6c73 into master Aug 30, 2017

aduth deleted the remove/crypto-dependency branch August 30, 2017 17:38

aduth mentioned this pull request Aug 30, 2017

Framework: Remove component-uid dependency #17560

Closed

matticbot removed the [Status] Ready to Merge label Aug 30, 2017

ockham added a commit that referenced this pull request Sep 21, 2017

Framework: Remove component-uid dependency

1fc9c26

Obsolete as of #17356.

ockham mentioned this pull request Sep 21, 2017

Framework: Remove component-uid dependency #18159

Merged

ockham added a commit that referenced this pull request Sep 21, 2017

Framework: Remove component-uid dependency (#18159)

fb33db9

Obsolete as of #17356.

aduth mentioned this pull request Sep 26, 2017

Collaborative Editing using WebRTC ( Only 2 peers support ) WordPress/gutenberg#1877

Closed

14 tasks

samouri mentioned this pull request Feb 21, 2018

Webpack: strictly verify imports and disable nodejs polyfilling #22630

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Framework: Substitute crypto polyfill dependants with smaller stand-ins #17356

Framework: Substitute crypto polyfill dependants with smaller stand-ins #17356

aduth commented Aug 18, 2017 •

edited

Loading

matticbot commented Aug 18, 2017

gwwar commented Aug 19, 2017

aidvu commented Aug 19, 2017 •

edited

Loading

retrofox Aug 19, 2017

aduth Aug 21, 2017

DanReyLop Aug 29, 2017

retrofox Aug 19, 2017

aduth Aug 21, 2017

matticbot commented Aug 21, 2017

blowery commented Aug 22, 2017

matticbot commented Aug 29, 2017

alisterscott commented Aug 30, 2017

aduth commented Aug 30, 2017

gwwar commented Aug 30, 2017

gwwar left a comment

aduth commented Aug 30, 2017

tyxla commented Aug 30, 2017

Framework: Substitute crypto polyfill dependants with smaller stand-ins #17356

Framework: Substitute crypto polyfill dependants with smaller stand-ins #17356

Conversation

aduth commented Aug 18, 2017 • edited Loading

matticbot commented Aug 18, 2017

gwwar commented Aug 19, 2017

aidvu commented Aug 19, 2017 • edited Loading

retrofox Aug 19, 2017

Choose a reason for hiding this comment

aduth Aug 21, 2017

Choose a reason for hiding this comment

DanReyLop Aug 29, 2017

Choose a reason for hiding this comment

retrofox Aug 19, 2017

Choose a reason for hiding this comment

aduth Aug 21, 2017

Choose a reason for hiding this comment

matticbot commented Aug 21, 2017

blowery commented Aug 22, 2017

matticbot commented Aug 29, 2017

alisterscott commented Aug 30, 2017

aduth commented Aug 30, 2017

gwwar commented Aug 30, 2017

gwwar left a comment

Choose a reason for hiding this comment

aduth commented Aug 30, 2017

tyxla commented Aug 30, 2017

aduth commented Aug 18, 2017 •

edited

Loading

aidvu commented Aug 19, 2017 •

edited

Loading