postcss / postcss-cli

CLI for postcss
Other
836 stars 93 forks source link

Check loaded postcss version #361

Closed ai closed 3 years ago

ai commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

Yeap, I like this plan

ai commented 3 years ago

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

thinkverse commented 3 years ago

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 commented 3 years ago

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

thinkverse commented 3 years ago

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

RyanZim commented 3 years ago

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