Fix ESLint and prettier integration for VS Code

[manzoorwanijk]

Fixes #20996

Changes proposed in this Pull Request:

• This PR makes .prettierrc.js in root directory to be a generic one
• Moves Svelte specific prettier config file to plugins/boost
• Fixes ESLint integration by using @rushstack/eslint-patch as a workaround.

Jetpack product discussion

p9dueE-4E8-p2

Does this pull request change what data or activity we track or use?

No

Testing instructions:

Before following the steps in VS Code, please ensure that you

• Have VS Code ESLint extension and Prettier for VS Code active.
• Have these settings in settings.json (Cmd + Shift + P > Preferences: Open Settings (JSON))

  "[javascript][javascriptreact][typescript][typescriptreact]": {
    "editor.formatOnSave": false,
    "editor.defaultFormatter": "esbenp.prettier-vscode"
  },
  "editor.codeActionsOnSave": {
    "source.fixAll.eslint": true,
    "source.fixAll.tslint": true,
    "source.fixAll.stylelint": true
  },

Steps

• Boot up the PR and run pnpm install
• Reload VS Code (Cmd + Shift + P > Developer: Reload Window)
• Open any file inside plugins/jetpack/_inc/client/ e.g. projects/plugins/jetpack/_inc/client/at-a-glance/activity.jsx
• Mess up the formatting. For example select some text and run Outdent line command (Cmd + [) a few times.
• Or add too many empty lines to the file, remove some trailing bracket spaces etc.
• Open problems view (Cmd + Shift + P > Problems: Focus on Problems View)
• Confirm that you see the problems reported by ESLint.
• Save Changes (Cmd + S)
• Confirm that the file is formatted as expected

[matticbot]

Caution: This PR has changes that must be merged to WordPress.com
Hello manzoorwanijk! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer and confirm D77357-code works as expected before merging this PR. Once this PR is merged, please commit the changes to WP.com. Thank you!
This revision will be updated with each commit to this PR

[github-actions]

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

• ✅ Include a description of your PR changes.
  
• ⚠️ All commits were linted before commit.
  
• ✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
  
• ✅ Add testing instructions.
  
• ✅ Specify whether this PR includes any changes to data or privacy.
  
• ✅ Add changelog entries to affected projects
  

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖

---

The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available.

---

Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped.
Then, add the "[Status] Needs Team review" label and ask someone from your team review the code.
Once you’ve done so, switch to the "[Status] Needs Review" label; someone from Jetpack Crew will then review this PR and merge it to be included in the next Jetpack release.

---

Jetpack plugin:

• Next scheduled release: April 5, 2022.
• Scheduled code freeze: March 29, 2022.

---

Boost plugin:

• Next scheduled release: April 5, 2022.
• Scheduled code freeze: March 28, 2022.

---

Starter Plugin plugin:

• Next scheduled release: April 5, 2022.
• Scheduled code freeze: March 28, 2022.

---

Protect plugin:

• Next scheduled release: April 5, 2022.
• Scheduled code freeze: March 28, 2022.

[manzoorwanijk]

This is to fix no-use-before define error for JetpackTerminationDialog. Weird that it showed the error though 🤷‍♂️

[anomiex]

Moves Svelte specific prettier config file to plugins/boost

I suppose the question is whether we think svelte will be used anywhere else, or if it'll remain a Boost-only thing.

[manzoorwanijk]

I suppose the question is whether we think svelte will be used anywhere else, or if it'll remain a Boost-only thing.

If we use Svelte anywhere else in future, then we can extract Svelte specific tools to js-tools and re-use from there.

[anomiex]

I note this drops a lot of warnings along the lines of

  181:0   warning  Invalid JSDoc @param "options" type "Object"; prefer: "object"             jsdoc/check-types

due to setting .settings.jsdoc.mode to "typescript". That change seems odd, but I find myself not really caring. If someone else does care, they're welcome to raise the question.

[anomiex]

P.S. I went ahead and filed WordPress/gutenberg#39810 asking them to make @wordpress/eslint-plugin's plugin dependencies into peer dependencies upstream.

[github-actions]

Great news! One last step: head over to your WordPress.com diff, D77357-code, and commit it.
Once you've done so, come back to this PR and add a comment with your changeset ID.

Thank you!

[manzoorwanijk]

I note this drops a lot of warnings along the lines of

  181:0   warning  Invalid JSDoc @param "options" type "Object"; prefer: "object"             jsdoc/check-types

due to setting .settings.jsdoc.mode to "typescript". That change seems odd

.settings.jsdoc.mode has been removed in #23522 because that uses @typescript-eslint/parser which uses that option implicitly.

[jeherve]

I don't seem to be able to build the monorepo now; I get the following errors:

devDependencies:
+ husky 7.0.4
+ jetpack-cli 1.0.0 <- tools/cli
+ jetpack-js-tools <- tools/js-tools

 ERR_PNPM_PEER_DEP_ISSUES  Unmet peer dependencies

projects/plugins/boost/tests/e2e
└─┬ eslint-plugin-playwright
  └── ✕ missing peer eslint@>=7
Peer dependencies that should be installed:
  eslint@>=7

projects/plugins/jetpack/tests/e2e
└─┬ eslint-plugin-playwright
  └── ✕ missing peer eslint@>=7
Peer dependencies that should be installed:
  eslint@>=7

projects/plugins/search/tests/e2e
└─┬ eslint-plugin-playwright
  └── ✕ missing peer eslint@>=7
Peer dependencies that should be installed:
  eslint@>=7

tools/e2e-commons
└─┬ eslint-plugin-playwright
  └── ✕ missing peer eslint@>=7
Peer dependencies that should be installed:
  eslint@>=7

tools/js-tools
├─┬ eslint-plugin-prettier
│ └── ✕ unmet peer prettier@>=2.0.0: found 2.2.1-beta-1
└─┬ prettier-plugin-svelte
  └── ✕ unmet peer prettier@"^1.16.4 || ^2.0.0": found 2.2.1-beta-1

Am I missing something?

[manzoorwanijk]

I don't seem to be able to build the monorepo now; I get the following errors:

May be because of #23456?

[kraftbj]

Thanks y'all for working together on this one.

@jeherve I can't duplicate the issue on my end (building plugins/jetpack from master completes without error).

[jeherve]

r242716-wpcom

[anomiex]

I don't seem to be able to build the monorepo now; I get the following errors:

Those shouldn't be a problem thanks to .pnpm.peerDependencyRules.ignoreMissing in the monorepo root packahe.json being set to include "eslint". Which version of pnpm are you on? Is there anything else that might be relevant?

[jeherve]

Which version of pnpm are you on?

This was the problem. Everything works after updating PNPM. Thank you!

[anomiex]

Which version of pnpm are you on?

This was the problem. Everything works after updating PNPM. Thank you!

Do we need to update the minimum version in the "engines" section of package.json?

[jeherve]

I cannot quite tell what version I was running before I updated. I should have checked, sorry!

[thingalon]

This change seems to have broken the Jetpack Boost pre-commit hooks:

❯ git commit -m "Commit tiny change"
Prettier formatting staged file: projects/plugins/boost/app/assets/src/js/pages/settings/elements/Module.svelte
[error] No parser could be inferred for file: projects/plugins/boost/app/assets/src/js/pages/settings/elements/Module.svelte
node:child_process:903
    throw err;
    ^

Error: Command failed: pnpx prettier --config .prettierrc.js --ignore-path .eslintignore --write projects/plugins/boost/app/assets/src/js/pages/settings/elements/Module.svelte
[error] No parser could be inferred for file: projects/plugins/boost/app/assets/src/js/pages/settings/elements/Module.svelte

    at checkExecSyncError (node:child_process:826:11)
    at execSync (node:child_process:900:15)
    at runPrettier (/Users/thingalon/Projects/jetpack/tools/js-tools/git-hooks/pre-commit-hook.js:165:3)
    at Object.<anonymous> (/Users/thingalon/Projects/jetpack/tools/js-tools/git-hooks/pre-commit-hook.js:433:2)
    at Module._compile (node:internal/modules/cjs/loader:1101:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1153:10)
    at Module.load (node:internal/modules/cjs/loader:981:32)
    at Function.Module._load (node:internal/modules/cjs/loader:822:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:81:12)
    at node:internal/main/run_main_module:17:47 {
  status: 2,
  signal: null,
  output: [
    null,
    Buffer(0) [Uint8Array] [],
    Buffer(125) [Uint8Array] [
       91, 101, 114, 114, 111, 114,  93,  32,  78, 111,  32, 112,
       97, 114, 115, 101, 114,  32,  99, 111, 117, 108, 100,  32,
       98, 101,  32, 105, 110, 102, 101, 114, 114, 101, 100,  32,
      102, 111, 114,  32, 102, 105, 108, 101,  58,  32, 112, 114,
      111, 106, 101,  99, 116, 115,  47, 112, 108, 117, 103, 105,
      110, 115,  47,  98, 111, 111, 115, 116,  47,  97, 112, 112,
       47,  97, 115, 115, 101, 116, 115,  47, 115, 114,  99,  47,
      106, 115,  47, 112,  97, 103, 101, 115,  47, 115, 101, 116,
      116, 105, 110, 103,
      ... 25 more items
    ]
  ],
  pid: 26027,
  stdout: Buffer(0) [Uint8Array] [],
  stderr: Buffer(125) [Uint8Array] [
     91, 101, 114, 114, 111, 114,  93,  32,  78, 111,  32, 112,
     97, 114, 115, 101, 114,  32,  99, 111, 117, 108, 100,  32,
     98, 101,  32, 105, 110, 102, 101, 114, 114, 101, 100,  32,
    102, 111, 114,  32, 102, 105, 108, 101,  58,  32, 112, 114,
    111, 106, 101,  99, 116, 115,  47, 112, 108, 117, 103, 105,
    110, 115,  47,  98, 111, 111, 115, 116,  47,  97, 112, 112,
     47,  97, 115, 115, 101, 116, 115,  47, 115, 114,  99,  47,
    106, 115,  47, 112,  97, 103, 101, 115,  47, 115, 101, 116,
    116, 105, 110, 103,
    ... 25 more items
  ]
}
husky - pre-commit hook exited with code 1 (error)

I'm on pnpm 6.32.3:

projects/plugins/boost on  text/pre-commit [+] is 📦 v1.4.2-alpha via  v16.13.2 via 🐘 v8.0.15 
❯ pnpm -v
6.32.3

I'll try to figure out how / why, but any help appreciated :)

[thingalon]

I've found the issue -- the pre-commit hooks explicitly use the root .prettierrc.js file, breaking checks for svelte files in Jetpack Boost.

I think I have a fix here to be reviewed: #23691

[manzoorwanijk]

@anomiex

As post-merge testing. I found a minor issue that needs to be addressed.

ESLint for VS Code and other IDEs look for eslint package inside node_modules in root directory. If it's not found in root directory, they keep looking for it inside every directory leading to the directory which contains the current file (being checked). If eslint is found no where in the path leading to the file directory, ESLint won't work, but there will be no error either. If will work only for the directories where eslint is installed anywhere in the parent directories.

For example if eslint is installed as a dependency inside plugins/jetpack, it will work only for the files in that directory. It won't work for js-packages, even if we install it for js-tools.

Solution: There are two possible solutions:

1. Install eslint as a dependency in each and every package/plugin that we want to use it for.
2. Install it in root package.json and remove it from all other packages.

CC @Automattic/jetpack-crew

[anomiex]

ESLint for VS Code and other IDEs look for eslint package inside node_modules in root directory.

Too bad they don't find the shim that we ensure is installed at the root's node_modules/.bin/eslint.

Install eslint as a dependency in each and every package/plugin that we want to use it for.

We don't want that, we want things centralized. Especially since we don't actually run eslint in the individual projects, only globally.

Install it in root package.json and remove it from all other packages.

Installing only eslint in the root would be ok-ish. Where it becomes a problem is if we would also have to install all the eslint configs and plugins and their peer dependencies there too. We don't want things like @babel/core that should be dependencies of individual projects being undeclared because node finds them at the monorepo root so no one realizes they're missing.

Even putting just eslint in the monorepo root would mean any of our own eslint plugin packages could forget a needed dep. But we don't have or plan to have many of those.

[manzoorwanijk]

Where it becomes a problem is if we would also have to install all the eslint configs and plugins and their peer dependencies there too. We don't want things like @babel/core that should be dependencies of individual projects being undeclared because node finds them at the monorepo root so no one realizes they're missing.

No, we just need eslint to be installed at root. All the plugins can stay in js-tools because we now use @rushstack/eslint-patch to load those from wherever we want.