is-shallow-equal: Convert to ESM #26833
Conversation
sarayourfriend
added
[Type] Enhancement
|
Size Change: -16 B (0%) Total Size: 1.19 MB
ℹ️ View Unchanged
|
|
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 |
|
I checked 2 failing E2E jobs for mobile apps and it looks like those are valid issues to resolve. |
|
@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: I assumed the issue here was a double I added a |
| "main": "lib/index.js", | ||
| "main": "build/index.js", | ||
| "module": "build-module/index.js", | ||
| "react-native": "build/index", |
There was a problem hiding this comment.
Should this refer to src/index like e.g. a11y does?
gutenberg/packages/a11y/package.json
Line 25 in 2296afb
| "react-native": "build/index", | |
| "react-native": "src/index", |
| 'use strict'; | ||
|
|
||
| var keys = Object.keys; | ||
| const keys = Object.keys; |
There was a problem hiding this comment.
keys appears to be used twice, stylistic but I'd prefer to just write Object.keys twice 🙂
sarayourfriend
force-pushed
the
try/esm-is-shallow-equal
branch
from
f6283fa
to
bb3f0b4
Compare
|
@sirreal Good catch! I've updated it. There's no reason to distribute the benchmark and test files I don't believe. |
|
Package lock problems now, maybe a rebase 😕 |
sarayourfriend
force-pushed
the
try/esm-is-shallow-equal
branch
from
4db4cf0
to
809fa9b
Compare
|
I tried to run benchmarks as noted in the README file 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 benchmarkYou 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: 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): |
|
@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 |
| @@ -2,6 +2,12 @@ | |||
|
|
|||
| ## Unreleased | |||
|
|
|||
| ## 3.0.0 (2020-11-12) | |||
There was a problem hiding this comment.
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", | |||
There was a problem hiding this comment.
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 😃
There was a problem hiding this comment.
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
force-pushed
the
try/esm-is-shallow-equal
branch
from
b400ff9
to
7149f36
Compare
|
@sirreal Can you re-review this PR? It's blocked on your change request (which I addressed 🙂) |
Description
This PR converts
is-shallow-equalto 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.jsin 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
masterand this branch. Should I also update the benchmarks? Specifically these can be slower (onmaster):Checklist: