JS linting is failing on this PR since it seems the implementation is a little broken.

Linting uses prettier, but it isn't been defined in package.json as it's been done in Gutenberg.

I can create a separate PR to fix it or fix it in this PR itself whichever seems to be a better way.

@eclarke1 Can you please add a "needs review" label and add @felixarntz as a reviewer?

@kirtangajjar absolutely! All updated, thank you so much for reaching out.

@dainemawer @mitogh Thank you for the feedback! I have made the changes. Please have a look at it again.

@kirtangajjar about #177 (comment) I guess the checks was failing because was missing the .eslintignore file with this content:

*.min.js
node_modules

@erikyo I think that's happening because we haven't added prettier in package.json https://github.com/WordPress/performance/runs/5303982931?check_suite_focus=true#step:8:13.

P.S. Gutenberg has it too.

that strange since prettier is a direct dependency of @wordpress/eslint-plugin

the "lint-js": "wp-scripts lint-js ./bin", is targeting this who is looking for eslint files and that the reason why i guess so.

@kirtangajjar this is a working setup for actions inspired to the performance repo i've done today, and the only dependency is the "@wordpress/scripts": "^21.0.2"

maybe isn't working because the outdated wpscript version? prettier is a dependency of multiple modules and to me it is not the cause of the failed test

@erikyo As you can see above, I just updated the version of @wordpress/scripts as you mentioned and it didn't solve the issue. Even on my local I nuked node_modules and I reinstalled it and it didn't install prettier. Perhaps because of this error that was thrown in the console:

npm WARN [email protected] requires a peer of prettier@>=1.13.0 but none is installed. You must install peer dependencies yourself.

As per their blog

We will also be changing the behaviour of peerDependencies in npm@3. We won’t be automatically downloading the peer dependency anymore. Instead, we’ll warn you if the peer dependency isn’t already installed. This requires you to resolve peerDependency conflicts yourself, manually, but in the long run, this should make it less likely that you’ll end up in a tricky spot with your packages’ dependencies.

My guess is this is why Gutenberg has prettier explicitly in its dependency too for this exact same reason.

I have used wp-prettier instead of prettier as it's a fork of prettier which is also used by Gutenberg. The changes made by WP team is documented in its readme:

This is a fork of Prettier that adds a new command line option --paren-spacing which inserts many extra spaces inside parentheses, the way how projects in the WordPress ecosystem (Calypso, Gutenberg, etc.) like to format their code.

I found out that the current implementation wasn't working for me in my local env. The problem was that we were trying to run the lint command inside of the container with husky and lint-staged.

When we commit any change, husky runs the lint-staged command, which runs the specified command, but only on the files that are changed. So the command becomes wp-env run composer run-script lint '/home/kirtan/performance.local/app/public/wp-content/plugins/performance/load.php'. This command is being run inside the container so the container will try to find the path to file inside the container and will throw an error at this point as that path is not present inside the container. It's only present on the host.

To remediate this, I think the best solution is to just run the command directly on the host. I think it's fair to assume most people will have composer and phpcs installed globally. So I have gone ahead with the change for now.

@mitogh @dainemawer LMK if you have any feedback regarding this.

I think that's a fair assumption @kirtangajjar, however I tested this locally and the husky command was not running when I staged some files.

When running the script npx lint-stagged the following output is returned:

Configuration could not be found.

Yeah agreed, I think thats solid. Nice work @kirtangajjar !

@mitogh

the husky command was not running when I staged some files.

The husky command will run after you commit anything(because of the pre-commit hook). However, for testing purposes, staging some changes and running npx lint-staged will have the same effect.

When running the script npx lint-stagged the following output is returned:

Configuration could not be found.

That's really strange. The configuration of lint-staged is present in package.json. Are you sure that you checked out the correct branch?

I performed the following steps in a newly cloned repo and it was running fine for me:

git clone https://github.com/WordPress/performance
git checkout -b feature/add-husky
git pull https://github.com/kirtangajjar/performance.git feature/add-husky
# Change admin/load.php
git add --all
npx lint-staged

I tried these same steps on a remote machine as well and it worked there too.

The husky command will run after you commit anything(because of the pre-commit hook)

Then this is a post-commit hook, not a pre-commit as the title of this PR indicates, from my perspective, this should run before the commit to ensuring all the appropriate lints are executed before a commit. @kirtangajjar

Looks like had to pull out from your fork as your branch was not found locally, the linting happen correctly before the commit and when running the standalone script lint-stagged.

So all good from my end @kirtangajjar

@felixarntz Thank you. The merge conflicts have been resolved!

@kirtangajjar Apologies, this just now got a second approval and there is now another merge conflict to resolve due to the version number change. @eugene-manuilov has rightfully raised this in #252 so that this won't happen again in the future.