feat: check postcss versions to avoid using PostCSS 7 #527

shlyk · 2021-05-15T12:13:52Z

This PR contains a:

Motivation / Use-Case

Added code that helps the developer understand that it is necessary to use an explicit version of PostCSS. Closes #507.

Breaking Changes

No breaking changes.

Additional Info

cc @ai

ai · 2021-05-15T12:35:53Z

LGTM

@alexander-akait can you start CI?

alexander-akait · 2021-05-15T12:37:32Z

src/index.js

+  // Check postcss versions to avoid using PostCSS 7
+  if (!isCheckedPostCSSVersion && postcss().version.startsWith("7.")) {
+    isCheckedPostCSSVersion = true;
+    const pkg = readPackageJson();


It will add one more fs call for each css file, reduce perf very well

As I understand, fs call will be only once and only for postcss 7

We can solve it on two ways:

webpack already reads package.json, need look where we store it

using this.fs, so each call will be cacheable

But it is invalid, in monorepos you can have multiple packages with different versions of postcss

@alexander-akait can you show examples of such repositories? I'm talking about the mono repository.

ai · 2021-05-15T12:57:16Z

Previous discussion about this problem #507

alexander-akait · 2021-05-15T13:14:38Z

We will positive false/false positive for monorepos and will spam warnings, it is bad, also we need more strong check here, we can read css file node_modules, we should not try to read package json in this case.

ai · 2021-05-15T13:18:28Z

Anyway we will run this check only for PostCSS 7 case, which will be rare case.

Should we add deep search and check for package.json? Another option is an environment variable to disable check in rare cases.

alexander-akait · 2021-05-15T13:28:58Z

15638766 downloads by week https://www.npmjs.com/package/postcss, it is big number, can't say how many uses potcss-loader, but I think it is big number too.

Should we add deep search and check for package.json? Another option is an environment variable to disable check in rare cases.

To be honest, this is not an easy task, you can see the algorithm in npm/yarn/pnpm and found it is not easy. Also it is not make sense for yarn PnP/pnpm (will be failed by default) and npm v7 (automatically install peer deps)

ai · 2021-05-15T13:41:29Z

15638766 downloads by week https://www.npmjs.com/package/postcss, it is big number, can't say how many uses potcss-loader, but I think it is big number too.

A lot of PostCSS 7 downloads is coming from plugins. The real number is much smaller.

Right now using PostCSS 7 is insecure because of vulnerability. It forced migration.

To be honest, this is not an easy task, you can see the algorithm in npm/yarn/pnpm and found it is not easy.

We do not need to cover all cases here. We just need to prevent popular error. Edge cases can be fixed by:

We can have an environment variable to disable check
We can use warning instead of error.

Also it is not make sense for yarn PnP/pnpm (will be failed by default) and npm v7 (automatically install peer deps)

I think npm 7 do not install peer dependencies for old dependencies on npm 6→7 migration. It will help only for new project or during installing new postcss-loader.

alexander-akait · 2021-05-15T13:47:36Z

I think npm 7 do not install peer dependencies for old dependencies on npm 6→7 migration. It will help only for new project or during installing new postcss-loader.

https://github.blog/2021-02-02-npm-7-is-now-generally-available/#peer-dependencies

ai · 2021-05-15T13:51:17Z

Since many packages in the ecosystem have come to rely on loose peer dependencies resolutions, npm 7 will print a warning and work around most peer conflicts that exist deep within the package tree, since you can’t fix those anyway.

Yeap. As I understand, npm 7 will not fix old missed peer dependencies.

shlyk · 2021-05-15T13:56:56Z

Please help me understand what to do next. Add a warning and an environment variable? Or do you want to do this task more thoroughly in another PR?

alexander-akait · 2021-05-15T13:58:26Z

Please help me understand what to do next. Add a warning and an environment variable? Or do you want to do this task more thoroughly in another PR?

I understand your excitement, but it's okay, we just need to discuss a little more and as soon as we come to a decision I immediately ping you

alexander-akait · 2021-05-15T14:09:51Z

Yeap. As I understand, npm 7 will not fix old missed peer dependencies.

npm 7 install them by default, no matter whether it is old or new

Anyway the main problem is not npm/yarn/pnpm, searching package.json is the main problem.

I think we should use, simplified, but covered most of cases:

const cwd = process.cwd();
let dir = cwd;
for (;;) {
    try {
        // `this` - is loader context
        if (this.fs.statSync(path.join(dir, 'package.json')).isFile()) break;
    } catch (e) {}
    const parent = path.dirname(dir);
    if (dir === parent) {
      dir = undefined;
      break;
    }
    dir = parent;
}

It's just that from my experience these things usually create more problems than they solve... Package manager output is the place where you need read warnings/errors. All of these workarounds are trying to train the developer to be attentive, but if you do not read errors from your tty, you may need to learn more.. Just my opinion.

ai · 2021-05-15T14:20:47Z

yarn output is full on warnings about deprecated nested dependencies, which you can't fix. They hide important warning which you can fix.

We know that people ignore this warning. Speaking about “the right way” will not solve the problem.

Another way to fix the problem, is to catch errors from PostCSS and show warning about missed dependency before re-throwing the error.

alexander-akait · 2021-05-15T14:40:10Z

Another way to fix the problem, is to catch errors from PostCSS and show warning about missed dependency before re-throwing the error.

Sounds very good, I like it

ai · 2021-05-15T14:44:30Z

@shlyk a new plan:

Wrap postcss.process can in try catch
On error from postcss we are starting old check
Instead of throwing error, we print warning

shlyk · 2021-05-15T15:08:04Z

@shlyk a new plan:

Wrap postcss.process can in try catch

On error from postcss we are starting old check

Instead of throwing error, we print warning

OK

shlyk · 2021-05-15T18:42:18Z

@ai and @alexander-akait, please check it out.

ai · 2021-05-15T22:42:30Z

src/index.js

+      postcssFactory().version.startsWith("7.")
+    ) {
+      // For caching reasons, we use the readFileSync function from the context, not the function from the `fs` module.
+      const pkg = readPackageJson(this.fs.readFileSync);


You need to check that pacjage.json exists first (print nothing on no file).

Otherwise, it will throw Can’t read file error if package.json was in a different place.

Thanks, done.

ai · 2021-05-17T00:16:17Z

src/index.js

+    // not the functions from the `fs` module.
+    if (
+      !hasExplicitDependencyOnPostCSS &&
+      isFileExists(PACKAGE_JSON_PATH, this.fs.existsSync) &&


Run postcssFactory().version.startsWith("7.") before isFileExists to avoid unnecessary file check for PostCSS 8 users

I also changed it to call statSync(...).isFile() instead of existsSync(), because existsSync() is not available in this version of Webpack.

ai · 2021-05-17T00:16:44Z

src/utils.js

@@ -408,10 +409,24 @@ function normalizeSourceMapAfterPostcss(map, resourceContext) {
  return newMap;
 }

+function isFileExists(filePath, existsSync = fs.existsSync) {


Do we need default value for any cases?

You're right, I removed the default value. They don't seem to be needed here.

ai · 2021-05-17T11:49:41Z

@alexander-akait we are ready for your review

alexander-akait · 2021-05-17T12:21:55Z

src/index.js

 } from "./utils";

+let hasExplicitDependencyOnPostCSS = false;
+
+const PACKAGE_JSON_PATH = path.resolve(process.cwd(), "package.json");


cwd can be src (i.e. src/package.json), but it is invalid I provided example above with recusing searching

it would be possible to use iterators to search for a file (from similar solutions), but it seems that this is not essential here. I took your solution, especially since there is a similar solution in the Webpack source code.

I also use null and undefined to cache calls. For some, this may seem implicit. I can try to make it more explicit, if necessary.

shlyk · 2021-05-20T15:51:48Z

@alexander-akait and @ai, are we doing something extra? Any other questions?

ai · 2021-05-20T15:53:38Z

LGTM. @alexander-akait let’s merge =^_^=

alexander-akait · 2021-05-20T15:54:33Z

I'm a little busy now, tomorrow I will merge and do release 👍

codecov · 2021-05-20T15:55:07Z

Codecov Report

Merging #527 (833f56b) into master (0c6dcdc) will decrease coverage by 2.11%.
The diff coverage is 72.00%.

@@            Coverage Diff             @@
##           master     #527      +/-   ##
==========================================
- Coverage   97.04%   94.93%   -2.12%     
==========================================
  Files           5        5              
  Lines         271      296      +25     
  Branches       88       96       +8     
==========================================
+ Hits          263      281      +18     
- Misses          8       14       +6     
- Partials        0        1       +1

Impacted Files	Coverage Δ
src/utils.js	`92.18% <46.15%> (-3.35%)`	⬇️
src/index.js	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0c6dcdc...833f56b. Read the comment docs.

alexander-akait · 2021-05-21T19:31:08Z

@shlyk Can you accept CLA (link in github actions checks)?

shlyk · 2021-05-21T19:42:17Z

@alexander-akait done.

alexander-akait · 2021-05-22T13:20:17Z

You need to use the same email in commits, can you add it?

…ency on PostCSS

…e.json

shlyk · 2021-05-24T13:10:50Z

@alexander-akait done.

shlyk · 2021-06-02T08:08:23Z

@alexander-akait, it looks like something needs to be done. Is there anything I can do to help?

alexander-akait · 2021-06-02T11:37:59Z

@shlyk You still have commits without email (look at your commits above), can you fix it?

shlyk changed the title ~~feat: check postcss versions to avoid using PostCSS 7~~ feat: check postcss versions to avoid using PostCSS 7 cc @ai May 15, 2021

shlyk changed the title ~~feat: check postcss versions to avoid using PostCSS 7 cc @ai~~ feat: check postcss versions to avoid using PostCSS 7 May 15, 2021

alexander-akait marked this pull request as ready for review May 15, 2021 12:36

alexander-akait reviewed May 15, 2021

View reviewed changes

ai reviewed May 15, 2021

View reviewed changes

ai reviewed May 17, 2021

View reviewed changes

alexander-akait reviewed May 17, 2021

View reviewed changes

alexander-akait closed this May 22, 2021

alexander-akait reopened this May 22, 2021

Alexey Shlyk and others added 9 commits May 24, 2021 16:05

feat: check postcss versions to avoid using PostCSS 7

7a19ce3

feat: added a flag which is responsible for having an explicit depend…

e33c5d6

…ency on PostCSS

feat: use readFileSync function from Webpack

aae55df

feat: reverted snapshots

ba3446b

feat: print warning when the loader is failed

a4a7625

feat: there should be no warnings if the version is defined in packag…

4fd7cde

…e.json

feat: check the package.json file before the main check

216f462

feat: removed unnecessary default values in utils functions

eac5f85

feat: search the package.json file in the current and parent directories

833f56b

alexander-akait merged commit 570db67 into webpack-contrib:master Jun 10, 2021

mcous mentioned this pull request Sep 7, 2021

Throws if either dependencies or devDependencies is not defined in package.json #547

Closed

feat: check postcss versions to avoid using PostCSS 7 #527

feat: check postcss versions to avoid using PostCSS 7 #527

Conversation

shlyk commented May 15, 2021 • edited

Motivation / Use-Case

Breaking Changes

Additional Info

ai commented May 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ai commented May 15, 2021 • edited

alexander-akait commented May 15, 2021 • edited

ai commented May 15, 2021 • edited

alexander-akait commented May 15, 2021

ai commented May 15, 2021 • edited

alexander-akait commented May 15, 2021

ai commented May 15, 2021

shlyk commented May 15, 2021

alexander-akait commented May 15, 2021

alexander-akait commented May 15, 2021

ai commented May 15, 2021

alexander-akait commented May 15, 2021

ai commented May 15, 2021

shlyk commented May 15, 2021

shlyk commented May 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ai commented May 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shlyk May 17, 2021 • edited

Choose a reason for hiding this comment

shlyk commented May 20, 2021

ai commented May 20, 2021

alexander-akait commented May 20, 2021

codecov bot commented May 20, 2021 • edited

Codecov Report

alexander-akait commented May 21, 2021

shlyk commented May 21, 2021

alexander-akait commented May 22, 2021

shlyk commented May 24, 2021

shlyk commented Jun 2, 2021 • edited

alexander-akait commented Jun 2, 2021

shlyk commented May 15, 2021 •

edited

ai commented May 15, 2021 •

edited

alexander-akait commented May 15, 2021 •

edited

ai commented May 15, 2021 •

edited

ai commented May 15, 2021 •

edited

shlyk May 17, 2021 •

edited

codecov bot commented May 20, 2021 •

edited

shlyk commented Jun 2, 2021 •

edited