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

Update is-shallow-equal to use ES5 code as before #8132

Merged
merged 2 commits into from
Jul 23, 2018

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented Jul 23, 2018

Description

Closes #8048.

Fixes an issue raised by @nerrad in #8048:

The docs for this package describe being able to import two specific exports for is-shallow-equal:

You can import a specific implementation if you already know the types of values you are working with:

import isShallowEqualArrays from '@wordpress/is-shallow-equal/arrays';
import isShallowEqualObjects from '@wordpress/is-shallow-equal/objects';

However, with the latest release of the package this no longer works. When building using webpack, the build fails with:

ERROR in ./src/index.js
Module not found: Error: Can't resolve '@wordpress/is-shallow-equal/arrays'

I followed recommendation from @aduth:

The root cause of this is that the require semantics were expecting arrays.js and objects.js files in the root module directory. These no longer exist (they live in build), as apparently the package is now subject to the transpile build step (it was not originally).

and reverted the change I introduce when merging this package into Gutenber repository in #7556.

How has this been tested?

  • npm test passes
  • both npm run dev and npm run build work as before

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@gziolo gziolo added [Type] Bug An existing feature does not function as intended [Type] Code Quality Issues or PRs that relate to code quality [Package] is-shallow-equal /packages/is-shallow-equal labels Jul 23, 2018
@gziolo gziolo added this to the 3.4 milestone Jul 23, 2018
@gziolo gziolo self-assigned this Jul 23, 2018
@gziolo gziolo requested review from nerrad and aduth July 23, 2018 10:33
Copy link
Contributor

@nerrad nerrad 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 (aside from the linting issue with package.json). I copied the changes into the @wordpress/is-shallow-equal package in my dummy app importing it and there's no build errors now.

"files": [
"arrays.js",
"objects.js"
],
Copy link
Contributor

Choose a reason for hiding this comment

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

package.json linting failed with this:

./packages/is-shallow-equal/package.json
[0] ✖ prefer-property-order - node:  - Your package.json properties are not in the desired order. Please move "main" after "files".

Copy link
Member Author

Choose a reason for hiding this comment

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

Ha, correct. Our linter is very strict 😄

@gziolo gziolo merged commit 5f69fc8 into master Jul 23, 2018
@gziolo gziolo deleted the update/is-shallow-equal branch July 23, 2018 13:59
@@ -0,0 +1,5 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

Note for future reference in extracting rules: We should consider having separate rulesets for ES5 vs. ES2015+ .

Looks like this is already built-in to eslint-plugin-wordpress:

https://github.com/WordPress-Coding-Standards/eslint-plugin-wordpress#available-rulesets

(Confused why there's both a config and a plugin which includes rulesets)

Copy link
Member Author

@gziolo gziolo Jul 23, 2018

Choose a reason for hiding this comment

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

Agreed, it would be much easier to put the following:

{
    "root": true,
    "extends": "something"
}

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I do this in my own projects, where (like with this one) I sometimes intentionally author in ES5.

https://github.com/aduth/memize/blob/4c0c9f9/.eslintrc.json#L3
https://github.com/aduth/equivalent-key-map/blob/e80140c/.eslintrc.json#L3

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] Bug An existing feature does not function as intended [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix unexpected behaviour for @wordpress/is-shallow-equal package.
3 participants