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

Check loaded postcss version #361

Closed
ai opened this issue Dec 6, 2020 · 9 comments
Closed

Check loaded postcss version #361

ai opened this issue Dec 6, 2020 · 9 comments

Comments

@ai
Copy link
Member

ai commented Dec 6, 2020

There are popular UX issue:

  1. User install postcss-cli, but forgot to install postcss
  2. postcss-cli takes old postcss 6 or postcss 7 from the deep dependencies
  3. User have true is not a PostCSS plugin without a good explanation

@RyanZim we can solve this problem by checking PostCSS version in postcss-cli:

if (parseInt(postcss().version) < 8) {
  throw new Error('Please install postcss or update it')
}
@thinkverse
Copy link
Contributor

Would this then disable postcss-cli from working unless the proper postcss version is installed?
Also, where would be a good place for this check to be.
In the first resolve where it checks for watch mode support? After that but before the file check?
Personal preference would be to check and exit as early as possible.

@ai
Copy link
Member Author

ai commented Dec 6, 2020

Would this then disable postcss-cli from working unless the proper postcss version is installed?

Yeap. The last major postcss-cli now has postcss in peerDependencies.

Also, where would be a good place for this check to be.

Hm. We can do it in the beggining of CLI after --version and --help protcessing.

@thinkverse
Copy link
Contributor

Hm. We can do it in the beggining of CLI after --version and --help protcessing.

That would be right before the Promise begins then? Since I'm guessing the processing your talking about is the argv section above it. That could be a good place, then it would fail after the plugins check which would be good.

Then the error experience would be something like: "Cannot find module -> Please install PostCSS 8 -> Cannot write in watch mode ...". Which is a nice DX IMHO, it tells a nice story.

@ai
Copy link
Member Author

ai commented Dec 6, 2020

Yeap, I like this plan

@ai
Copy link
Member Author

ai commented Dec 6, 2020

Do you want to send PR? (Sorry, I am preparing PostCSS 8.2 today)

@thinkverse
Copy link
Contributor

Seems like an easy enough PR, question is, how would one test unsupported postcss versions. 🤔
I'll look inside tests to see if I can find an idea of that.

@ai
Copy link
Member Author

ai commented Dec 6, 2020

I am personally OK for /* istanbul ignore next */ for errors like this. Hope Ryan agree.

@thinkverse
Copy link
Contributor

Submitted a PR, have some issues testing the thrown error though. 🤔

@RyanZim
Copy link
Collaborator

RyanZim commented Dec 8, 2020

Yeah, you'd hope npm peerDependency warnings would get people's attention, but unfortunately, it doesn't look like it's working.

@RyanZim RyanZim closed this as completed in 96b6521 Dec 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants