Add @woocommerce/eslint-plugin

[nerrad]

This pull adds a @woocommerce/eslint-plugin to publish for usage by WooCommerce projects. Notably, the configuration has the following:

• convention and standards are defined by @woocommerce/eslint-plugin/recommended' (which aligns with WordPress JavaScript standards)
• implements prettier configuration for formatting from the above package.
• registers wcSettings as a global
• implements radix as error rule (parseInt must include the radix value).
• disallows using "yoda" conditions (with 'error').
• extends react-hooks/recommended (which gives more complete hooks coverage over what is included with the @wordpress/eslint-plugin (significantly, this adds warning for the exhaustive-deps rule).
• adds a custom rule for WooCommerce dependencies. For now this just requires an "External dependencies" block and an "Internal dependencies" block (in that order if present). There's been some discussion on whether to implement a `WooCommerce dependencies" block and I've conservatively excluded that with this pull submission as I don't think it's necessary. Can be modified in another iteration if necessary.
• adds the react-testing-library eslint-plugin configured for JavaScript unit tests.

Notes

• Consumers of this package shouldn't need to install wp-prettier, however, it had to be installed in the top level package.json for wc-admin because the storybook package installs the canonical prettier (not the wp-prettier fork) which overrides the nested wp-prettier package imported by the new eslint-plugin via @wordpress/eslint-plugin. When we publish the initial version of this package, I'm hoping that testing in the blocks repo reveals this works as I expect.
• Implementing this revealed a LOT of eslint issues (including calling out some potentially fragile/buggy code). I started going through and cleaning up some of the obvious ones but figured it be better to do that in smaller pulls. So I left what I had done, but then implemented changing rules in the .eslintrc to be warnings instead of errors. That way your team can go through and do one pull for each rule to clean up files related to that rule (just remove the rule line so it goes back to being "error" and then fix all the errors). I should note, that the first five rules there revealed some pretty significant issues, so I would prioritize fixing those.

Next steps

• Smoke test wc-admin in this branch to make sure nothing obvious is broken (i.e. you can do both npm run start and npm run build without errors and run the plugin without new introduced problems).
• Merge to main
• Publish the package
• Create pulls to deal with each of the rules in .eslintrc.js so that you can remove it and rely on it throwing errors. The sooner this is done the better because as I pointed out, the first 5 especially are significant if they are being caught by the linter.
• Once the package is published, I'll test in the blocks project to see if it works as expected and create pull(s) for any necessary fixes if needed.
• Once verified the package is working as expected, we'll announce for woo teams!

[nerrad]

With this present I got an Error: Duplicate plugin/preset detected. when trying to execute a build (npm run start). Removing this line fixed the build. It's not clear to me why this is essentially a duplicate of what is included in @wordpress/babel-preset-default but it may be related to the fact that wp-scripts got an update in this pull which may in turn pull in a more recent version of @wordpress/babel-preset-default.

[psealock]

Seems likely. wp-scripts went from 3.4.0 to 12.0.0. I'm not sure how Renovate let that happen.

[nerrad]

These commits were necessary to fix the following error I was getting on builds:

ERROR in ./client/index.js 2:9
Module parse failed: Identifier 'createElement' has already been declared (2:9)
File was processed with these loaders:
 * ./node_modules/babel-loader/lib/index.js
You may need an additional loader to handle the result of these loaders.
| import { createElement } from "@wordpress/element";
> import { createElement } from "@wordpress/element";
| 
| /**

(there were three files producing that error).

[nerrad]

Looks like all tests pass now except for the PHP 5.6 unit tests. Which is... puzzling because this pull doesn't really touch anything to do with PHP. Based on the failure it looks like it may be a datetime related bug, so likely unrelated to anything in this pull.

[psealock]

Consumers of this package shouldn't need to install wp-prettier, however, it had to be installed in the top level package.json

I think thats preferable because IDE integrations are setup to look for it in node_modules

[psealock]

Thanks @nerrad for getting this going. I smoke tested wc-admin and added a few quick eslint fixes to see how it was working. Nice work!

All the changes proposed in this PR match what was discussed previously and I feel good merging this. I think one more ✅ from someone in regards to running through the app to make sure no errors were introduced would be sufficient to get this merged.

[nerrad]

I think thats preferable because IDE integrations are setup to look for it in node_modules

Right, but with some preliminary testing just switching the blocks repo to use the @wordpress/eslint-plugin package and removing wp-prettier from the blocks package.json import it seemed to work fine in my IDE. So I'm hoping this package will work too without it when it's imported in there. I'm looking forward to trying it out :)

[nerrad]

I forgot to add the eslint plugin for react-testing-library, so the latest commit adds that.

[psealock]

@nerrad, if I remember correctly, you use this IDE prettier formatter for VSC https://marketplace.visualstudio.com/items?itemName=esbenp.prettier-vscode.

Are you able to use the Ctr + Shift + P function without a .prettierrc file?

[psealock]

I couldn't figure out how to configure the VSCode plugin so in 4bafa7c I instead opted for adding .prettierrc back in and pointing to https://github.com/WordPress/gutenberg/tree/master/packages/prettier-config.

I tested a the IDE functionality and formatting a commit.

[psealock]

Something odd is going on when building the app. Its like the postCSS is broken because theme(button) isn't a color.

It is, however, working on main branch. I know $theme-color has been deprecated, but I don't understand why this branch would pull in a different version of base-styles.

[nerrad]

I couldn't figure out how to configure the VSCode plugin so in 4bafa7c I instead opted for adding .prettierrc back in and pointing to https://github.com/WordPress/gutenberg/tree/master/packages/prettier-config.

Hmm odd, I have my IDE setup to format on save and I didn't need to have a prettier config file in place to have things "just work". I do have the same extension installed you referenced too.

What might be different between how I have things configured and how you have things configured is I'm using the eslint fixer for format on save vs prettier format. I do notice that if I explicitly format a doc using prettier format it's not picking up the prettier config. Here's a slice of my config related to this:

{
  "[javascript]": {
    "editor.formatOnPaste": true,
    "editor.defaultFormatter": "esbenp.prettier-vscode",
  },
  "editor.codeActionsOnSave": {
    "source.fixAll.eslint": true
  },
}

We'll have to see how this plays out with the WooCommerce eslint-plugin installed as a package in other projects (starting with Woo Blocks) to see whether this is definitely needed (and document findings). AFAIK it should work "out of the box" without additional configuration.

[nerrad]

Something odd is going on when building the app. Its like the postCSS is broken because theme(button) isn't a color.

Ya it's a puzzler, does this branch need rebased on the main branch (is the reason things differ between this branch and main is because main has been updated to fix that)?

[psealock]

Rebasing this branch introduced the bug. Oddly enough, main hasn't updated base styles yet. I'm wondering if if has something to do with the postCSS package being removed.

[psealock]

Here's a slice of my config related to this:

Interesting, I can replicate and format on save without the config, but not the explicit Ctrl + Shift + P.

[psealock]

Ya it's a puzzler

We're working on updating base styles and removing postCSS separately, so if I can't find a fix soon, waiting for the update to be merged should resolve the issue.

[psealock]

#4759 removes postCSS. Hopefully that resolves the issue, which I can't get to the bottom of. Marking this as blocked in the meantime.

[psealock]

The rebase solved our postCSS woes 😄 , which is great because I couldn't find the culprit. Maybe it was a mismatched dependency.

@samueljseay Since this PR removes .prettierrc, would you like to confirm the fixes you recently made are also reflected here? That logic is now via prettier-config.

[psealock]

I will wait for the 1.4.x release branch to be cut before merging this one.

[psealock]

@nerrad do you know why this dependency isn't getting added on npm install? The lint step is failing.

ESLint couldn't find the plugin "@wordpress/eslint-plugin".

[nerrad]

I think the issue here is that with this being a monorepo, and @woocommerce/eslint-plugin being included in the top package.json via files:// (so you're always using the latest from the package in development), you have to hoist the dependencies of the new package to the root package.json as devDependencies so they are installed.

The important thing is to just make sure the versions match between the package and the root.

[nerrad]

Offhand my guess would be because npm install isn't being run directly, but instead this is being run:

npm run -s install-if-deps-outdated

If you switch to just do npm install in this shell script used by travis, does that fix?

[psealock]

Hmm, even npm install from the command line isn't working. I have tried different versions of npm. Other dependencies in packages/eslint-plugin/package.json are included just fine, even if you add another one from wordpress, ie something like@wordpress/nux. Something about @wordpress/eslint-plugin may not be right.

[psealock]

Some combination of npm install, deleting package-lock.json, removing node_modules from packages/eslint-plugin worked to produce a new package-lock.json this time. No idea what changed 🤷

[nerrad]

Yay! I think this is one of the challenges with npm and mono-repos. Thanks for finishing this off Paul!