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 postcss versions to avoid using PostCSS 7 #507

Closed
ai opened this issue Dec 30, 2020 · 13 comments · Fixed by #527
Closed

Check postcss versions to avoid using PostCSS 7 #507

ai opened this issue Dec 30, 2020 · 13 comments · Fixed by #527

Comments

@ai
Copy link
Contributor

ai commented Dec 30, 2020

Many users report the same problem with PostCSS 7 → 8 migration.

  1. They updated postcss-loader
  2. They ignored postcss peer dependency warning
  3. npm takes old postcss 7 from some deep dependency
  4. As a result they have an error about PostCSS incompatibility

Of course, we can blame users. But this is a popular error.

In postcss-cli we check postcss().version and show Add postcss to package.json error message. Should we do something like this in postcss-loader?

@alexander-akait
Copy link
Member

We allow to use PostCSS 7 and PostCSS 8, what the warhing we should show?

@ai
Copy link
Contributor Author

ai commented Dec 30, 2020

We can do something like this:

if (postcss().version.startsWith('7.')) {
  let pkg = readPackageJson()
  if (!pkg.dependencies.postcss && !pkg.devDependencies.postcss) {
    throw new Error('Add postcss as project dependency. postcss is not a peer dependency for postcss-loader. Use `npm install postcss` or `yarn add postcss`')
  }
}

@alexander-akait
Copy link
Member

But npm@7 install peers deps automatically, yarn exit with non zero code, I can accept this PR, but I don't understand why developers ignore messages from package manager

@ai
Copy link
Contributor Author

ai commented Dec 31, 2020

I don't understand why developers ignore messages from package manager

Short answer: because they always ignore warnings 😅

Long answer: npm writes a lot of messages and teach users to ignore them.

yarn exit with non zero code

Yarn 1 (which is still the most popular) just print warning.

npm@7 install peers deps automatically

npm 7 is a non-standard version. Only npm 8 will be LTS version.

I can accept this PR

  1. Do you like the idea of reading package.json to detect missed dependencies? I didn’t found a better way.
  2. Is it OK to add an extra dependency to read package.json? Do you have any preferences?

I will create task on Cult of Martians to prepare PR.

@alexander-akait
Copy link
Member

alexander-akait commented Jan 3, 2021

Do you like the idea of reading package.json to detect missed dependencies? I didn’t found a better way.

hm, extra fs call, not good for performance, maybe we can try to use require.resolve?

@ai
Copy link
Contributor Author

ai commented Jan 3, 2021

we can try to use require.resolve?

Can you show pseudo-code to understand what do you mean?

@alexander-akait
Copy link
Member

Move postcss from import and use try { require.resolve('postcss') } catch {}

@ai
Copy link
Contributor Author

ai commented Jan 4, 2021

If project has a few PostCSS plugins, npm can deduplicate postcss:

project/
  node_modules/
    postcss-plugin1/
    postcss-plugin2/
    postcss/

Am I right, that in this case require.resolve() will work (even if package.json has no postcss)?

@alexander-akait
Copy link
Member

If plugins have postcss in peer deps no postcss in node_modules and require.resolve throw an error

@ai
Copy link
Contributor Author

ai commented Jan 5, 2021

But most PostCSS plugins still have postcss in direct dependencies. I think we should expect to see PostCSS 7 for a few years.

We need a solution for this case. The case with PostCSS 8 only plugins is not a problem, since these users will have errors like postcss was not found, which is pretty self-explainable.

@alexander-akait
Copy link
Member

Searching and reading package.json (don't forget about monorepos and other non standard structures) is very expensive operation, we will very slow down compilation.

Honestly encourage bad practices like not reading messages from package managers, ignoring changelog when you updating new versions and etc are very bad ideas, many projects do it and in the future it will create huge problems with updates and performance

@ai
Copy link
Contributor Author

ai commented Jan 5, 2021

Honestly encourage bad practices like not reading messages from package managers, ignoring changelog when you updating new versions and etc are very bad ideas, many projects do it and in the future it will create huge problems with updates and performance

Users do not read docs and do not read ChangeLog. This is reality.

We can blame users, but blaming users will not make PostCSS 7→8 migration easier.

Searching and reading package.json is very expensive operation

Yeap, I know that it is a dirty thing. This is why I am suggesting to do it only for postcss 7. We can also do it only once (by using some global boolean mark).

If this way is OK, I will create a task on the Cult of Martians. If you have any other option, I am open to it.

I know that it is simpler for us to ignore that problem. But we need to find some solution. Twitter is full of people, who forget to install postcss.

@alexander-akait
Copy link
Member

Yeap, I know that it is a dirty thing. This is why I am suggesting to do it only for postcss 7. We can also do it only once (by using some global boolean mark).

Sounds good, PR welcome

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants