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

Add prettier dependency to @wordpress/eslint-plugin #20028

Merged
merged 2 commits into from
Feb 4, 2020

Conversation

kasparsd
Copy link
Contributor

@kasparsd kasparsd commented Feb 4, 2020

Description

Fixes #20027.

In #19963 we added eslint-plugin-prettier as a dependency to @wordpress/eslint-plugin in f5c44ea which requires prettier as a peer dependency.

How has this been tested?

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR. .

@@ -33,6 +33,7 @@
"eslint-plugin-react": "^7.14.3",
"eslint-plugin-react-hooks": "^1.6.1",
"globals": "^12.0.0",
"prettier": "npm:wp-prettier@1.19.1",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This matches what was used in #18048 for wp-scripts in d20fa45.

@ntwb ntwb added the [Tool] ESLint plugin /packages/eslint-plugin label Feb 4, 2020
@epiqueras
Copy link
Contributor

You need to commit lock-file changes.

@ntwb
Copy link
Member

ntwb commented Feb 4, 2020

I think we need to take some time to test the configuration combinations that @wordpress/scripts and @wordpress/eslint-plugin can be used.

See also 0a96d2c

Edit: Discussion is also on the agenda for todays #core-js chat in Slack shortly

@epiqueras
Copy link
Contributor

How so?

@kasparsd
Copy link
Contributor Author

kasparsd commented Feb 4, 2020

You need to commit lock-file changes.

@epiqueras I'm not seeing a lock file in the root directory of this package so I wasn't sure it's required. Please confirm.

@epiqueras
Copy link
Contributor

It's at the root of the repo (Lerna hoisting). Just run npm install.

@ntwb
Copy link
Member

ntwb commented Feb 4, 2020

The lock file needs to b updated using Lerna, not npm install

See: https://github.com/WordPress/gutenberg/tree/master/packages#adding-new-dependencies

Though I'm not sure how Lerna will handle the npm alias /shrug

lerna add npm:wp-prettier packages/eslint-plugin

@epiqueras
Copy link
Contributor

Both work.

@ntwb
Copy link
Member

ntwb commented Feb 4, 2020

Let's follow the docs please @epiqueras, Lerna has idiosyncratic behaviour that lends to having the lock file out of date, the docs were written to mitigate this scenario as much as possible from past expereinces

@kasparsd
Copy link
Contributor Author

kasparsd commented Feb 4, 2020

Lock file updated in 2026a60

@gziolo
Copy link
Member

gziolo commented Feb 4, 2020

It looks good as far as I can tell. In the lock, there is only one line updated which means the same version is shared 👍

@kasparsd, thanks for catching and proposing the fix. We didn't test ESLint plugin in isolation :)

@gziolo gziolo merged commit 3b00abf into WordPress:master Feb 4, 2020
@gziolo gziolo added this to the Gutenberg 7.4 milestone Feb 4, 2020
@kasparsd kasparsd deleted the fix/eslint-plugin-deps branch February 4, 2020 14:15
@epiqueras
Copy link
Contributor

Let's follow the docs please @epiqueras, Lerna has idiosyncratic behaviour that lends to having the lock file out of date, the docs were written to mitigate this scenario as much as possible from past expereinces

I thought we had a postinstall hook. If not, we should have one. We also need to correct that documentation to use the local version of Lerna.

Does that sound good? I can open a PR.

cc @aduth

epiqueras pushed a commit that referenced this pull request Feb 5, 2020
* The recommended ruleset now requires prettier

* Sync the lock file
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Tool] ESLint plugin /packages/eslint-plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error: Cannot find module 'prettier' with @wordpress/eslint-plugin v3.4.0
4 participants