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

Scripts: Prettier fork not found when running lint-js #21872

Open
renatho opened this issue Apr 24, 2020 · 17 comments
Open

Scripts: Prettier fork not found when running lint-js #21872

renatho opened this issue Apr 24, 2020 · 17 comments
Labels
[Status] Blocked Used to indicate that a current effort isn't able to move forward [Tool] WP Scripts /packages/scripts [Type] Bug An existing feature does not function as intended

Comments

@renatho
Copy link
Contributor

renatho commented Apr 24, 2020

Describe the bug

Error with the prettier dependency while running the lint script from @wordpress/scripts after a new npm install.

To reproduce
Steps to reproduce the behavior:

  1. Create a project and add the @wordpress/scripts dev dependency
> mkdir scripts-test \
&& cd scripts-test \
&& npm init -y && npm i -D @wordpress/scripts
  1. Add the script to the package.json: "lint:js": "wp-scripts lint-js"
  2. Create a js file and run the lint to see it is working.
> echo 'const x = 1;' >index.js \
&& npm run lint:js
  1. Checking the prettier dependency in this moment:
> npm ls prettier
scripts-test@1.0.0 /Users/renatho/Downloads/scripts-test
└─┬ @wordpress/scripts@7.1.3
  ├─┬ @wordpress/eslint-plugin@4.0.0
  │ └── prettier@npm:wp-prettier@1.19.1  deduped
  └── prettier@npm:wp-prettier@1.19.1
  1. You can make a git init here and commit the package-lock to see the next changes.
  2. Remove the node_modules folder, reinstall the packages and run the lint script. The new install will update (unexpectedly) our package-lock.json and It stops to work because it doesn't find the prettier. You will see the error Error: Cannot find module 'prettier'
> rm -rf node_modules \
&& npm i \
&& npm run lint:js
  1. Check the prettier dependency to understand what happened (you can also see the package-lock diff):
> npm ls prettier
scripts-test@1.0.0 /Users/renatho/Downloads/scripts-test
└─┬ @wordpress/scripts@7.1.3
  ├─┬ @wordpress/eslint-plugin@4.0.0
  │ └── UNMET PEER DEPENDENCY prettier@npm:wp-prettier@1.19.1 
  └── prettier@npm:wp-prettier@1.19.1

Expected behavior

Install the @wordpress/scripts and after other npm install it continues to work properly while running the npm run lint:js.

Version (please complete the following information):

  • A clean project with only the @wordpress/scripts installed. Reproduced in 7.1.3 and 8.0.1

Desktop (please complete the following information):

  • OS: macOS Catalina
  • NPM 6.14.4 / Node 10.17.0
@gziolo gziolo added [Tool] WP Scripts /packages/scripts [Type] Bug An existing feature does not function as intended labels Apr 26, 2020
@gziolo
Copy link
Member

gziolo commented Apr 26, 2020

It’s a know issue and I don’t think we can help much about it. In my experience running npm install twice resolves it, so it looks like there is something weird happening on the npm side when using aliases. There are two things that might help:

  • a troubleshooting section where we describe this issue and propose running npm i again when it happens or installing the aliased Prettier fork as a dependency of the project
  • stop using Prettier’s fork but we would have to change the coding guidelines about all the spaces added in the code, Prettier refused to add it as a configurable option

@renatho
Copy link
Contributor Author

renatho commented Apr 26, 2020

Thank you for your answer with the suggestions @gziolo !!

For now, what we are using as a workaround is to install the prettier fork as a direct dependency. :)

And I tested running the npm i again, but it didn't solve for me :(
For me, after the error happens changing the package-lock, it doesn't work more unless we remove the package-lock to reinstall.

@gziolo
Copy link
Member

gziolo commented Apr 26, 2020

@renatho thanks for testing and sharing what works for you 👍

tschortsch added a commit to liip/bootstrap-blocks-wordpress-plugin that referenced this issue Jun 20, 2020
tschortsch added a commit to liip/bootstrap-blocks-wordpress-plugin that referenced this issue Jun 21, 2020
* switch to using wp-env instead of wordpress/scripts for setting up local environment

* fix js code style

* remove unused install-wp script

* use wp-env in travis build

* add missing env variables

* add aliased wp-prettier as direct dependency (see: WordPress/gutenberg#21872)

* fix broken theme activation after installation

* use chmod command from gutenberg repository in travis ci

* fix theme on test instance

* temporary disable broken alignment tests

* fix js code style

* skip tests with skip function

* use install wordpress flag from gutenberg

* update composer dependencies

* fix plugin folder name in ci build

* disable alignment tests

* use correct phpunit script in ci
@ndiego
Copy link
Member

ndiego commented Nov 10, 2020

I just ran into this randomly as well. I had been using the lint command with no issues for awhile, and then all the sudden I got the error. If you remove package-lock.json file, delete the node_modules folder and then run npm i again, the error went away for me.

@gziolo
Copy link
Member

gziolo commented Dec 19, 2020

The best way to address this issue would be to stop using a npm alias for Prettier fork and use the official Prettier. However we would have to change WordPress coding styles for JavaScript to reflect how Prettier removes spaces around brackets.

@renatho
Copy link
Contributor Author

renatho commented Dec 21, 2020

The best way to address this issue would be to stop using a npm alias for Prettier fork and use the official Prettier. However we would have to change WordPress coding styles for JavaScript to reflect how Prettier removes spaces around brackets.

I imagine it would have a big impact. But it can be a good option for those who want to avoid this issue by replacing this standard, installing the dependency directly in their projects. :)

@gziolo
Copy link
Member

gziolo commented Dec 21, 2020

I imagine it would have a big impact. But it can be a good option for those who want to avoid this issue by replacing this standard, installing the dependency directly in their projects. :)

It only seems like a big impact on the vast majority of projects. If you don't have hundreds of open PRs in the repository then it all boils down to running wp-scripts format-js once and you are all set.

@gziolo gziolo changed the title Prettier not found running lint from @wordpress/scripts Scripts: Prettier fork not found when running lint-js Feb 1, 2021
@gziolo gziolo added the Needs Decision Needs a decision to be actionable or relevant label Feb 1, 2021
@ockham
Copy link
Contributor

ockham commented Mar 5, 2021

@jsnajdr @tyxla et al -- Have we seen any similar errors with the prettier fork in Calypso? If so, are there any known "permanent" fixes/workarounds?

@gziolo
Copy link
Member

gziolo commented Mar 8, 2021

They have it installed in the top-level package.json
https://github.com/Automattic/wp-calypso/blob/1718f43e78277d06f16df1bc550917dfb7d0c9c9/package.json#L281

Gutenberg does it, too. This resolves the issue but it shouldn't be necessary if one of the sub packages does it already. Another workaround is to run npm install twice, but it's very confusing.

@jsnajdr
Copy link
Member

jsnajdr commented Mar 8, 2021

scripts-test@1.0.0 /Users/renatho/Downloads/scripts-test
└─┬ @wordpress/scripts@7.1.3
  ├─┬ @wordpress/eslint-plugin@4.0.0
  │ └── UNMET PEER DEPENDENCY prettier@npm:wp-prettier@1.19.1 
  └── prettier@npm:wp-prettier@1.19.1

I'm pretty sure this is a bug in npm 6.x.x. The @wordpress/eslint-plugin package specifies prettier as a direct dependency, so why wasn't it installed again after rm node_modules and npm i?

A year ago, I was solving another npm bug where packages were mistakenly removed from the node_modules tree, because npm thought they were not needed. There it was happening for file: links: npm/cli#750

Apparently the code for package aliases has a similar bug. My impression is that the support for package aliases was added to npm as a quick-and-dirty patch and is not very reliable.

Running npm i twice or installing the package also to root are somewhat random workarounds that try to lead npm away from the buggy code path. Sometimes they work, sometimes they don't.

Upgrade to npm 7 will solve this hopefully.

@gziolo
Copy link
Member

gziolo commented Mar 8, 2021

Upgrade to npm 7 will solve this hopefully.

It would be awesome. Regardless, I plan to recommend that WordPress projects switch to the official Prettier and adjust coding style guidelines to align with what Prettier can support.

@chrisvanpatten
Copy link
Contributor

@gziolo that would be fantastic. It would make things so much easier… this bug has been persistently annoying 😅

@gziolo
Copy link
Member

gziolo commented Mar 23, 2021

My proposal needs to wait until we sort out how we want to use TypeScript in Gutenberg:
https://make.wordpress.org/core/2021/03/18/proposal-native-typescript-support-in-gutenberg/

@gziolo
Copy link
Member

gziolo commented Sep 10, 2021

The same issue was reported in #34622. The context was @wordpress/script but the solution was also installing the npm alias for Prettier in the project.

@kasparsd
Copy link
Contributor

I created this test repository https://github.com/xwp/gutenberg-issue-21872-scripts which attempts to isolate the issue and require just @wordpress/scripts at ^18.0.1 and runs npm install followed by wp-scripts lint-js using different versions of Node and different package lock files generated by different versions of Node. See the GitHub action output or the test matrix logs (as GitHub action reports expire after some time).

The results are all over the place as it appears to be an issue with Node and/or npm instead of these packages.

@gziolo
Copy link
Member

gziolo commented Sep 17, 2021

The results are all over the place as it appears to be an issue with Node and/or npm instead of these packages.

@kasparsd, thank you for doing a very in-depth investigation. That was my understanding, too. That's why I proposed we stop using the aliased fork and go with the official Prettier to save folks from all the hassle that it creates, even if that is independent from the WordPress packages.

mkaz pushed a commit to WordPress/gutenberg-examples that referenced this issue Sep 24, 2021
* Update @wordpress/scripts

* Update 01-basic-esnext to use block.json

* Add .nvmrc file.

* Add .editorconfig file.

* Add dependencies and package-json.lock

* Add common eslintrc file.

* Remove js map files

* Ignore dev js mapping files.

* Update main package.json to use latest version of the @wordpress/scripts package.

* Split out files and linting fixes.

* Inline comments to explain the loading process.

* Add build files.

* Add manual linting to avoid error with prettier not being found.

* Remove linting from ci due error with prettier not being found. See WordPress/gutenberg#21872

* Remove npm test command in CI

* Add node 14 to versions in CI
@gziolo
Copy link
Member

gziolo commented Dec 23, 2021

I opened #37607 to start making the official proposal to switch to the original Prettier in the most recent 2.5 version.

@gziolo gziolo added [Status] Blocked Used to indicate that a current effort isn't able to move forward and removed Needs Decision Needs a decision to be actionable or relevant labels Feb 11, 2022
goldentroll added a commit to goldentroll/gutenberg-examples that referenced this issue Mar 13, 2023
* Update @wordpress/scripts

* Update 01-basic-esnext to use block.json

* Add .nvmrc file.

* Add .editorconfig file.

* Add dependencies and package-json.lock

* Add common eslintrc file.

* Remove js map files

* Ignore dev js mapping files.

* Update main package.json to use latest version of the @wordpress/scripts package.

* Split out files and linting fixes.

* Inline comments to explain the loading process.

* Add build files.

* Add manual linting to avoid error with prettier not being found.

* Remove linting from ci due error with prettier not being found. See WordPress/gutenberg#21872

* Remove npm test command in CI

* Add node 14 to versions in CI
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Status] Blocked Used to indicate that a current effort isn't able to move forward [Tool] WP Scripts /packages/scripts [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants