Use WordPress Coding Standards and Scripts

[gregrickaby]

Closes #614

• Copy over all WordPress config files
• Remove WDS config files
• Refactor Webpack config(s)
• Copy over NPM Scripts and pass in --config flags
• Test dev mode
• Verify source-maps work in dev mode
• Verify Prettier when writing SCSS and JavaScript
• Verify Stylelint when writing SCSS
• Verify LiveReload
• Verify PHPCS via Composer
• Test production/build mode
• Verify asset minification
• Theme wide formatting changes with new standards
• Update README

DESCRIPTION

This PR introduces a nearly 1:1 sync with WordPress standards and tooling. The goal is to reduce internal maintenance of standards, documentation, and tooling and just leverage what WordPress already has in place.

• Remove WDS Coding Standards for JavaScript, SCSS, and CSS (PHP standards are still WDS)
• Remove WDS Scripts for @wordpress/scripts defaults
• Remove extra devDependencies by leveraging what already comes with @wordpress/scripts (Autoprefixer, PostCSS, etc...)
• Remove CSS Nano in PostCSS config in favor of css-minimizer-webpack-plugin (which uses CSS Nano)
• Add support for (WordPress's) Prettier
• Add support for markdown linting
• Add support for package.json linting
• Add support for better globbing and NPM script matching with Lefthook
• Add support for development vs. production modes (via separate webpack configs and ENV vars) to decrease dev build times (e.g., don't minify stuff in dev mode)

SCREENSHOTS

Initial dev build times using npm run dev are ~2.5 seconds 👍🏻

STEPS TO VERIFY

Note: Buddy assertions will fail until after this PR is merged and Buddy is updated!

Install

• Check out this PR
• Run rm -rf node_modules package-lock.json
• Run npm i --legacy-peer-deps

Develop

• Run npm run dev and make changes SCSS files and monitor build times.
• Run npm run build to create a production build and verify everything has been minified/optimized

POST-MERGE CHORES

• Update Buddy pipelines
• Update WDS Guide coding standards to just point at WordPress
• Remove WDS packages from NPM repo
• Update Audit docs
• Chat with @aubreypwd about ongoing maintenance for PHP standards (I know we want short array syntax, does this need to be a separate "package" or can it just be added to this repo's .phpcs.xml.dist instead? For context, I had to slightly modify the .eslintrc.js file because we don't use React. I think this is fine that it's in this repo, and doesn't require WDS to create a seperate NPM package.

[coreymcollins]

Thanks for this @gregrickaby!

It looks like this removes Browsersync – are we pulling that out again?

[gregrickaby]

@coreymcollins 😬 That was an oversight on my part. I'll get it added back in. Sorry!

[coreymcollins]

In doing some light testing, I've found some things that aren't happening right now:

• Language files are never created in the /build/ directory, even during npm run build
• The sprite.svg file isn't being created during npm run dev
  • If I change an SVG file while running this, the SVG file IS updated in build but the sprite is never built
• Browsersync starts, but we're not actually running start so nothing is being listened to/rebuilt

I think there are a couple of additional packages we'll need to install, if we want to keep the current workflow:

• npm-run-all
• onchange

npm-run-alll lets us use run-p (and run-s, which may be useful) so we can setup a start:sync type of script like we have running currently ("start:sync": "run-p start sync watch:icons",).

onchange helps us listen for changes in the src/image/icons directory so we can regenerate the sprite.svg file when something changes. Without the sprite, nothing will show up when using the SVG display PHP functions during dev.

I'm sure there's more than one way to do these things, so I wanted to note them all here so we can chat about the best route to go with these rather than hacking at your branch and making changes.

[gregrickaby]

@coreymcollins I apologize, I should have been more clear. The build just exits until the error is fixed. With Buddy, it would stop and throw an error in a Slack Firehose channel.

• On a "fresh" build, JS/CSS assets will not be generated and the script just exits. (POT file runs, because that's a composer script)
• On a "rebuild", assets that were successfully generated prior are not overwritten/deleted.

[aubreypwd]

• Proxying still doesn't work for me, I don't have any configuration that I don't think would conflict, I'm not sure why it doesn't work "on my machine."

More and more rules...

Added formatting commands to Lefthook. The tabs vs. spaces issues should be taken care of by Prettier before a commit is pushed**
Added declaration-no-imporant rule to .stylelintrc.json
Added camelCase rule to .eslintrc.js

It's great that we added these rules here, but this is where I was hoping "rebooting" coding standards to follow WP would take care of these (and others, if there are others). I find it odd that it's not taking care of these, I'm not sure I'm a fan of having to add all these caveats to config files... especially as they add up project to project (including wd_s here)...

...that also being said I did flag just these few things. I did not test every pattern (rule) I could think of that are in line with what our coding standards used to look like. Not sure what other rules have changed/don't work but it looks like they are adding up...

...that also being said I think this begs a couple questions:

1. Should we NOT tweak these things and simply allow what WP is doing guide us here, e.g. if it's cool with !important should we follow along?
2. If not (and we want to tweak them or it's a growing # of them e.g. not just one or two) I think this would warrant creating a package to add these tweaks vs using config files

Commit Hook or PR Check step?

There is an issue with Tower and git hooks. Tower doesn't use the shell, so it's not aware of PATH, therefore cannot find Lefthook

This has me worried. A lot of people use GUI clients...including me. I wonder if this means that maybe final formatting/linting needs to happen at the PR level (another option I've been mulling over since we decided to reboot CS): could we maybe do this linting/formatting at the PR level e.g. via something Buddy could run/etc vs. at the commit hook. E.g.:

• You commit code however you want, even code that's "bad", sure go ahead
• You have linting to guide you still, and if you like it, auto-formatting configs to help you out too on save, etc (but let's just say it's someone who doesn't like auto-formatting or linting)
• Once you open a PR it will run an check and this check will:
  • Try and auto-format your code for you on the files you changed (still not sure I'm a fan of this, but on new builds it may make more sense) and not bug you since we can fix it for you here
  • Run all the linting stuff afterwards (hopefully auto-formatting step caught a bunch of stuff for you)
  • If the linting steps at this point still do not checkout, report back to the PR that it doesn't pass the check and (if we can do it) have the PR marked back to Draft (so developers would have to see this check pass to submit the PR to us)

The tower issue changed my perspective a bit on controlling things when the developer develops (e.g. commit hooks). PR creation (vs the commit hook) could try and auto-format for you and check (lint/etc) your code there (in the PR check steps) vs while your developing. I'm an advocate of giving the developer more freedom on their computer and make it hard for developers to create a PR that doesn't follow our rules. Also I'm an advocate for people who like auto-formatting and want to use it while they are developing to help them along, but if you don't want to use it you don't have to—but then you have to fix your code before you can submit a PR (I'm one of those people who are willing to take on that task). It would use the same tools that are installed in the project that would report to your editor in linting and the same auto-formatting config you might also have on in your editor.

If all the checks and auto-formatting attempts (same thing we're trying to do on commit hooks) happen at a PR check step then we rid ourselves of having to make our configurations super-compatible with editor(s): Vim, Sublime, PHPStorm, etc vs. VSCode. Of course we would have tools in the project to help your editors out, but we don't have to worry about the person's computer being perfectly configured to get a PR created properly (e.g. a perfectly formatted PR). We can just (IMO) do that in a PR check step...

1. Try to auto-format it for you
2. Lint the final result

(All stuff we would do in commit hooks)

So if you have auto-formatting on and linting on in your editor, you're PR creation step should go super smooth. If you don't then you might have things to fix (but I anticipate most people will at least have linting on, but who knows).

What do you think? I know this is drastically different than doing this on commit hooks, but it has the same result: No submitted PR's that don't have formatting issues.

This gives the developer more freedom on their computer. E.g. I can have auto-formatting off (because I don't like it personally—but have linting on) and be able to work without having to figure out how this project wants my code to look. Or I can be the type of person who wants auto-formatting to just do it for me in my editor and I will adapt. When I create a PR it will auto-format my code and lint my code. If it checks out a PR is created, if not it's punted back to a Draft.

Someone like @itsamoreh could have linting and auto-formatting on in his editor, and write code according to the formatting we've configured in the project as he goes. He can get the benefits of auto-formatting and it's highly likely when he gets to the PR process he will have more success. Because he had auto-formatting on, his code will likely pass the steps the PR hook will do.

...if my idea here is even possible.

Caveats of my idea here:

• When the auto-formatter runs in the assertion step, it would have to be able to commit any changes for you and push it up to the branch you created (which I think could be possible)
• When this commit goes up to GH you would have to know to always update your branch e.g. a git pull to pull that auto-format attempt

And to be clear, I am suggesting we auto-format code in an assertion NOT in your editor (unless you want it to via auto-formatting and linting) and NOT in a commit hook. You can commit code just as you would, but to open a PR a check would have to pass where we would attempt to auto-format your code for you, and only if it fails then you would have to fix it yourself in your editor where things like npm run watch and npm run lint* tasks could be used to look at where you need to make corrections. If you have auto-formatting on in your editor (or at the very least follow linting) though you shouldn't have a problems getting a PR to pass...

[jrfoell]

Thanks for you work on this @gregrickaby

[aubreypwd]

Per our demo and chat today this is good to go! Great work Greg!

[salcode]

These notes are from commit 5b2e695

composer.lock

During npm i, I got the message

Warning: The lock file is not up to date with the latest changes in composer.json. You may be getting outdated dependencies. It is recommended that you run composer update or composer update <package name>.

indicating composer.lock is not up to date with composer.json

Running composer update did not introduce any changes (other than updating the content-hash in composer.lock), we can update this hash (and just this hash) with

composer update --lock

which will eliminate the warning message.

In Editor Linting Errors

In Vim, running the ALE plugin with the settings I already had in place, I'm seeing linting errors flagged but changes are not automatically applied.

e.g.

Exception to Flagging Errors

In src/scss/base/_headings.scss when I changed the indentation on a line from tabs to spaces, it was not flagged.

Other errors in the same file were flagged.

Linting on Commit

When I ran git commit the linting errors that could be automatically corrected were and my commit was rejected 👍

It was a little confusing that I then had both staged (my original changes) and unstaged (the linting fixes) changes. I think this is the best way to handle things but I can imagine this may be a stumbling point for those new to automated linting. We may want to call this out specifically when training team members to use this.

Unflagged Linting Error was Still Fixed

The SCSS linting error I introduced by changing an indent from tab to spaces that was not flagged in the editor (as mentioned earlier in this comment), was fixed during the correcting on commit.

This may be an issue in my editor failing to flag the space/tab problem 🤷‍♂️ .

Overall Impression

Overall, I think this is an impressive setup that should help team members avoid wasting time correcting linting errors. 👍

Great work!

[gregrickaby]

@salcode Thanks for the feedback and overall sign-off. 🥳

Tabs vs Spaces

This isn't a rule, but rather an opinion listed in both .editorconfig and .prettierrc.js. Therefore, they will never be "flagged" by any IDE as an error/warning. They will be re-formatted though-- when Prettier runs.

Vim

Unfortunately, I'm not knowledgeable enough about VIM or the plugin you mentioned to provide any assistance. I'm glad that the pre-commit hooks caught the issues and took care of them.

Demo

I'll be doing a demo on Tuesday's FEE/BEE scrum, and would happy to walk through the commit hook process. See ya then! 👍🏻

[gregrickaby]

@coreymcollins I updated the Assertions in Buddy and ran them against this PR. Green light! 🟢

I humbly await your sign-off so we can get this merged in.

Thanks!

[gregrickaby]

Just noting that Corey and I spoke in Slack and he's 👍🏻