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

is-shallow-equal: Convert to ESM #26833

Merged
merged 6 commits into from
Nov 13, 2020
Merged

Conversation

sarayourfriend
Copy link
Contributor

@sarayourfriend sarayourfriend commented Nov 9, 2020

Description

This PR converts is-shallow-equal to ES modules. I'm proposing this in order to allow it to be consumed in an ESM context. ES modules have matured in node to the point where this should be viable.

How has this been tested?

Original tests continue to pass. Build passes. Benchmark runs.

Types of changes

Breaking change. See webpack/webpack#4742 for details on how importing this module in CJS context needs to change. benchmark/index.js in this changeset is a good example of how this will change.

I actually noticed that the benchmark seems to be slower than the README. This is true on master and this branch. Should I also update the benchmarks? Specifically these can be slower (on master):

@wordpress/is-shallow-equal (object, equal) x 4,933,251 ops/sec ±0.76% (88 runs sampled)
@wordpress/is-shallow-equal (object, same) x 196,122,526 ops/sec ±1.04% (88 runs sampled)
@wordpress/is-shallow-equal (object, unequal) x 5,450,480 ops/sec ±1.03% (85 runs sampled)
@wordpress/is-shallow-equal (array, equal) x 26,996,443 ops/sec ±1.01% (88 runs sampled)
@wordpress/is-shallow-equal (array, same) x 61,463,049 ops/sec ±0.96% (89 runs sampled)
@wordpress/is-shallow-equal (array, unequal) x 28,314,907 ops/sec ±0.79% (91 runs sampled)

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.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@sarayourfriend sarayourfriend added [Type] Enhancement A suggestion for improvement. [Type] Build Tooling Issues or PRs related to build tooling [Package] is-shallow-equal /packages/is-shallow-equal labels Nov 9, 2020
@sarayourfriend sarayourfriend self-assigned this Nov 9, 2020
@github-actions
Copy link

github-actions bot commented Nov 9, 2020

Size Change: -16 B (0%)

Total Size: 1.19 MB

Filename Size Change
build/block-directory/index.js 8.71 kB -1 B
build/compose/index.js 9.9 kB -1 B
build/core-data/index.js 14.8 kB +1 B
build/data/index.js 8.74 kB -1 B
build/dom/index.js 4.92 kB -1 B
build/edit-navigation/index.js 11.1 kB +1 B
build/edit-widgets/index.js 26.3 kB +1 B
build/editor/index.js 42.5 kB -1 B
build/format-library/index.js 6.86 kB -1 B
build/i18n/index.js 3.57 kB -1 B
build/is-shallow-equal/index.js 698 B -15 B (2%)
build/keyboard-shortcuts/index.js 2.52 kB +2 B (0%)
build/list-reusable-blocks/index.js 3.1 kB +1 B
build/notices/index.js 1.77 kB -2 B (0%)
build/primitives/index.js 1.43 kB +1 B
build/redux-routine/index.js 2.84 kB +5 B (0%)
build/rich-text/index.js 13.3 kB -1 B
build/server-side-render/index.js 2.77 kB -1 B
build/token-list/index.js 1.27 kB -1 B
build/url/index.js 4.05 kB -1 B
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.77 kB 0 B
build/api-fetch/index.js 3.42 kB 0 B
build/autop/index.js 2.84 kB 0 B
build/blob/index.js 664 B 0 B
build/block-directory/style-rtl.css 943 B 0 B
build/block-directory/style.css 942 B 0 B
build/block-editor/index.js 133 kB 0 B
build/block-editor/style-rtl.css 11.1 kB 0 B
build/block-editor/style.css 11.1 kB 0 B
build/block-library/editor-rtl.css 8.91 kB 0 B
build/block-library/editor.css 8.91 kB 0 B
build/block-library/index.js 147 kB 0 B
build/block-library/style-rtl.css 8.1 kB 0 B
build/block-library/style.css 8.1 kB 0 B
build/block-library/theme-rtl.css 792 B 0 B
build/block-library/theme.css 793 B 0 B
build/block-serialization-default-parser/index.js 1.87 kB 0 B
build/block-serialization-spec-parser/index.js 3.06 kB 0 B
build/blocks/index.js 48 kB 0 B
build/components/index.js 171 kB 0 B
build/components/style-rtl.css 15.3 kB 0 B
build/components/style.css 15.3 kB 0 B
build/data-controls/index.js 821 B 0 B
build/date/index.js 11.2 kB 0 B
build/deprecated/index.js 768 B 0 B
build/dom-ready/index.js 571 B 0 B
build/edit-navigation/style-rtl.css 881 B 0 B
build/edit-navigation/style.css 885 B 0 B
build/edit-post/index.js 306 kB 0 B
build/edit-post/style-rtl.css 6.43 kB 0 B
build/edit-post/style.css 6.42 kB 0 B
build/edit-site/index.js 23.3 kB 0 B
build/edit-site/style-rtl.css 3.95 kB 0 B
build/edit-site/style.css 3.95 kB 0 B
build/edit-widgets/style-rtl.css 3.16 kB 0 B
build/edit-widgets/style.css 3.16 kB 0 B
build/editor/editor-styles-rtl.css 476 B 0 B
build/editor/editor-styles.css 478 B 0 B
build/editor/style-rtl.css 3.85 kB 0 B
build/editor/style.css 3.85 kB 0 B
build/element/index.js 4.62 kB 0 B
build/escape-html/index.js 735 B 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/hooks/index.js 2.16 kB 0 B
build/html-entities/index.js 623 B 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/style-rtl.css 476 B 0 B
build/list-reusable-blocks/style.css 476 B 0 B
build/media-utils/index.js 5.32 kB 0 B
build/nux/index.js 3.4 kB 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/priority-queue/index.js 790 B 0 B
build/reusable-blocks/index.js 3.05 kB 0 B
build/shortcode/index.js 1.69 kB 0 B
build/viewport/index.js 1.84 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.22 kB 0 B

compressed-size-action

@gziolo
Copy link
Member

gziolo commented Nov 10, 2020

At some point, we converted this package to ESM but we reverted it back to ES5 because of some breaking changes introduced. See: #8132.

It shouldn't be an issue anymore, now that #18942 landed with a new major version that removed this limitation.

Some tools might still assume that is-shallow-equal is ES5 based, so we will have to update that as well. Although, I couldn't find any place that would be affected. It looks like we made configuration for tools more flexible :)

@gziolo
Copy link
Member

gziolo commented Nov 10, 2020

I checked 2 failing E2E jobs for mobile apps and it looks like those are valid issues to resolve.

@sarayourfriend
Copy link
Contributor Author

@gziolo Any ideas how to address this? At first I thought it was merely a resolution issue as the original error stated something along the lines of:

Unable to resolve ".../index.js(index.js|.js|...)"

I assumed the issue here was a double .js.

I added a react-native entry to the package.json to match other packages I observed doing the same thing, but that did not resolve the issue 🤔 Any help would be appreciated!

"main": "lib/index.js",
"main": "build/index.js",
"module": "build-module/index.js",
"react-native": "build/index",
Copy link
Member

Choose a reason for hiding this comment

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

Should this refer to src/index like e.g. a11y does?

"react-native": "src/index",

Suggested change
"react-native": "build/index",
"react-native": "src/index",

'use strict';

var keys = Object.keys;
const keys = Object.keys;
Copy link
Member

Choose a reason for hiding this comment

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

keys appears to be used twice, stylistic but I'd prefer to just write Object.keys twice 🙂

@sarayourfriend sarayourfriend force-pushed the try/esm-is-shallow-equal branch from f6283fa to bb3f0b4 Compare November 11, 2020 18:21
Copy link
Member

@sirreal sirreal left a comment

Choose a reason for hiding this comment

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

files in package.json needs to be updated:

https://github.com/WordPress/gutenberg/blob/bb3f0b4ee0c4673a940ab089ee886770c88b99ce/packages/is-shallow-equal/package.json#L23-L27

These are pesky because you don't catch them until you've published a broken package. I know from experience 😅

Maybe remove it entirely?

@sarayourfriend
Copy link
Contributor Author

@sirreal Good catch! I've updated it. There's no reason to distribute the benchmark and test files I don't believe.

@sirreal sirreal self-requested a review November 11, 2020 20:28
@sirreal
Copy link
Member

sirreal commented Nov 11, 2020

Package lock problems now, maybe a rebase 😕

@sarayourfriend sarayourfriend force-pushed the try/esm-is-shallow-equal branch from 4db4cf0 to 809fa9b Compare November 11, 2020 22:03
@gziolo
Copy link
Member

gziolo commented Nov 12, 2020

I tried to run benchmarks as noted in the README file
(https://github.com/WordPress/gutenberg/blob/master/packages/is-shallow-equal/README.md#benchmarks)

It needs to be updated to include the step that builds packages:

cd packages/packages/is-shallow-equal
npm install
npm run build:packages
node benchmark

You might see the following issue when you run the benchmark for the first time:

(node:65044) UnhandledPromiseRejectionWarning: Error: Cannot find module '@wordpress/lazy-import.5ab856344c555e50779f56b4cd1f2e02'

It's a known issue. All subsequent runs should work as expected.

There is an issue with importing code using CJS:

https://github.com/WordPress/gutenberg/blob/master/packages/is-shallow-equal/benchmark/index.js#L43-L44

This code probably errors as I no longer see benchmark results:

@wordpress/is-shallow-equal (object, equal): 
@wordpress/is-shallow-equal (object, same): 
@wordpress/is-shallow-equal (object, unequal): 
@wordpress/is-shallow-equal (array, equal): 
@wordpress/is-shallow-equal (array, same): 
@wordpress/is-shallow-equal (array, unequal):

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

See my previous comment, otherwise, it's good to go.

@sarayourfriend
Copy link
Contributor Author

@gziolo Thanks for catching that. I've fixed the issue and noted that there is now a breaking change (really glad you tried out the benchmark).

I also updated the description to note that the benchmarks are different from the README's. This is true of master as of this branch (though there is no significant difference between this branch and master). Shall I also update the README with new benchmarks?

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

It would be nice to update benchmark's results. It might be a follow up PR if other libraries listed are outdated.

@@ -2,6 +2,12 @@

## Unreleased

## 3.0.0 (2020-11-12)
Copy link
Member

Choose a reason for hiding this comment

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

Release tool will add a version number and date. It's all automated based on type of sections listed.

@@ -1,6 +1,6 @@
{
"name": "@wordpress/is-shallow-equal",
"version": "2.3.0",
"version": "3.0.0",
Copy link
Member

Choose a reason for hiding this comment

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

Here as well, it should remain unchanged. Lerna will bump version at the time of npm publishing.

How does it work in Calypso, I'm curious 😃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I should have read the link at the top of the file 😉 In Calypso these are maintained manually as far as I know!

@sarayourfriend sarayourfriend force-pushed the try/esm-is-shallow-equal branch from b400ff9 to 7149f36 Compare November 12, 2020 23:04
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

It is good to go, thank you 😃

@sarayourfriend
Copy link
Contributor Author

@sirreal Can you re-review this PR? It's blocked on your change request (which I addressed 🙂)

Copy link
Member

@sirreal sirreal 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, thanks.

@sarayourfriend sarayourfriend merged commit a109fda into master Nov 13, 2020
@sarayourfriend sarayourfriend deleted the try/esm-is-shallow-equal branch November 13, 2020 15:26
@github-actions github-actions bot added this to the Gutenberg 9.4 milestone Nov 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] is-shallow-equal /packages/is-shallow-equal [Type] Build Tooling Issues or PRs related to build tooling [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants