Skip to content
This repository has been archived by the owner on Oct 24, 2023. It is now read-only.

Move postcss to peerDependencies #65

Closed
stalniy opened this issue Aug 19, 2018 · 35 comments
Closed

Move postcss to peerDependencies #65

stalniy opened this issue Aug 19, 2018 · 35 comments

Comments

@stalniy
Copy link

stalniy commented Aug 19, 2018

postcss-preset-env is a plugin for postcss thus should include postcss as peerDependency and not as depedency in package.json. This basically allows to use whatever supported version of postcss together with the plugin and be sure that different postcss plugins use the same instance of postcss.

Please read this article https://nodejs.org/en/blog/npm/peer-dependencies/ for more details.

I can create a PR if you are ok

@pascalduez
Copy link

pascalduez commented Aug 19, 2018

Hi,

I was wondering the same thing with all those PostCSS 7 upgrades. Also we had a similar case somehow in postcss/postcss-reporter#44.
The thing is every PostCSS plugins set postcss as dependencies, so it's not just about this one particularly.
I would be interested to know the reasoning (if any) that led to this while implementing the first plugins. /cc @ai
Also sometimes peerDependencies can be a pain.

@stalniy
Copy link
Author

stalniy commented Aug 19, 2018

npm starting from 3.x doesn't install peerDepdencies, so I don't see the reason why it can be a pain. Could you please clarify this.

Yes, every postcss plugin includes postcss as depedency. I think this is a mistake. Because eventually if you write npm ls in your package you will see that different plugins uses different versions of postcss what may result in hidden issues.

This is output from my project
├─┬ cssnano@4.0.5
│ ├─┬ cssnano-preset-default@4.0.0
│ │ ├─┬ css-declaration-sorter@3.0.1
│ │ │ └── postcss@6.0.23 
│ │ ├─┬ cssnano-util-raw-cache@4.0.0
│ │ │ └── postcss@6.0.23 
│ │ ├── postcss@6.0.23 
│ │ ├─┬ postcss-calc@6.0.1
│ │ │ └── postcss@6.0.23 
│ │ ├─┬ postcss-colormin@4.0.1
│ │ │ └── postcss@6.0.23 
│ │ ├─┬ postcss-convert-values@4.0.0
│ │ │ └── postcss@6.0.23 
│ │ ├─┬ postcss-discard-comments@4.0.0
│ │ │ └── postcss@6.0.23 
│ │ ├─┬ postcss-discard-duplicates@4.0.0
│ │ │ └── postcss@6.0.23 
│ │ ├─┬ postcss-discard-empty@4.0.0
│ │ │ └── postcss@6.0.23 
│ │ ├─┬ postcss-discard-overridden@4.0.0
│ │ │ └── postcss@6.0.23 
│ │ ├─┬ postcss-merge-longhand@4.0.4
│ │ │ ├── postcss@6.0.23 
│ │ │ └─┬ stylehacks@4.0.0
│ │ │   └── postcss@6.0.23 
│ │ ├─┬ postcss-merge-rules@4.0.1
│ │ │ └── postcss@6.0.23 
│ │ ├─┬ postcss-minify-font-values@4.0.0
│ │ │ └── postcss@6.0.23 
│ │ ├─┬ postcss-minify-gradients@4.0.0
│ │ │ └── postcss@6.0.23 
│ │ ├─┬ postcss-minify-params@4.0.0
│ │ │ └── postcss@6.0.23 
│ │ ├─┬ postcss-minify-selectors@4.0.0
│ │ │ └── postcss@6.0.23 
│ │ ├─┬ postcss-normalize-charset@4.0.0
│ │ │ └── postcss@6.0.23 
│ │ ├─┬ postcss-normalize-display-values@4.0.0
│ │ │ └── postcss@6.0.23 
│ │ ├─┬ postcss-normalize-positions@4.0.0
│ │ │ └── postcss@6.0.23 
│ │ ├─┬ postcss-normalize-repeat-style@4.0.0
│ │ │ └── postcss@6.0.23 
│ │ ├─┬ postcss-normalize-string@4.0.0
│ │ │ └── postcss@6.0.23 
│ │ ├─┬ postcss-normalize-timing-functions@4.0.0
│ │ │ └── postcss@6.0.23 
│ │ ├─┬ postcss-normalize-unicode@4.0.0
│ │ │ └── postcss@6.0.23 
│ │ ├─┬ postcss-normalize-url@4.0.0
│ │ │ └── postcss@6.0.23 
│ │ ├─┬ postcss-normalize-whitespace@4.0.0
│ │ │ └── postcss@6.0.23 
│ │ ├─┬ postcss-ordered-values@4.0.0
│ │ │ └── postcss@6.0.23 
│ │ ├─┬ postcss-reduce-initial@4.0.1
│ │ │ └── postcss@6.0.23 
│ │ ├─┬ postcss-reduce-transforms@4.0.0
│ │ │ └── postcss@6.0.23 
│ │ ├─┬ postcss-svgo@4.0.0
│ │ │ └── postcss@6.0.23 
│ │ └─┬ postcss-unique-selectors@4.0.0
│ │   └── postcss@6.0.23 
│ └── postcss@6.0.23 
├─┬ postcss-cli@6.0.0
│ ├── postcss@7.0.1 
│ └─┬ postcss-reporter@5.0.0
│   └── postcss@6.0.23 
├─┬ postcss-critical-css@3.0.1
│ ├─┬ cssnano@3.10.0
│ │ ├─┬ autoprefixer@6.7.7
│ │ │ └── postcss@5.2.18 
│ │ ├── postcss@5.2.18 
│ │ ├─┬ postcss-calc@5.3.1
│ │ │ └── postcss@5.2.18 
│ │ ├─┬ postcss-colormin@2.2.2
│ │ │ └── postcss@5.2.18 
│ │ ├─┬ postcss-convert-values@2.6.1
│ │ │ └── postcss@5.2.18 
│ │ ├─┬ postcss-discard-comments@2.0.4
│ │ │ └── postcss@5.2.18 
│ │ ├─┬ postcss-discard-duplicates@2.1.0
│ │ │ └── postcss@5.2.18 
│ │ ├─┬ postcss-discard-empty@2.1.0
│ │ │ └── postcss@5.2.18 
│ │ ├─┬ postcss-discard-overridden@0.1.1
│ │ │ └── postcss@5.2.18 
│ │ ├─┬ postcss-discard-unused@2.2.3
│ │ │ └── postcss@5.2.18 
│ │ ├─┬ postcss-filter-plugins@2.0.3
│ │ │ └── postcss@5.2.18 
│ │ ├─┬ postcss-merge-idents@2.1.7
│ │ │ └── postcss@5.2.18 
│ │ ├─┬ postcss-merge-longhand@2.0.2
│ │ │ └── postcss@5.2.18 
│ │ ├─┬ postcss-merge-rules@2.1.2
│ │ │ └── postcss@5.2.18 
│ │ ├─┬ postcss-minify-font-values@1.0.5
│ │ │ └── postcss@5.2.18 
│ │ ├─┬ postcss-minify-gradients@1.0.5
│ │ │ └── postcss@5.2.18 
│ │ ├─┬ postcss-minify-params@1.2.2
│ │ │ └── postcss@5.2.18 
│ │ ├─┬ postcss-minify-selectors@2.1.1
│ │ │ └── postcss@5.2.18 
│ │ ├─┬ postcss-normalize-charset@1.1.1
│ │ │ └── postcss@5.2.18 
│ │ ├─┬ postcss-normalize-url@3.0.8
│ │ │ └── postcss@5.2.18 
│ │ ├─┬ postcss-ordered-values@2.2.3
│ │ │ └── postcss@5.2.18 
│ │ ├─┬ postcss-reduce-idents@2.4.0
│ │ │ └── postcss@5.2.18 
│ │ ├─┬ postcss-reduce-initial@1.0.1
│ │ │ └── postcss@5.2.18 
│ │ ├─┬ postcss-reduce-transforms@1.0.4
│ │ │ └── postcss@5.2.18 
│ │ ├─┬ postcss-svgo@2.1.6
│ │ │ └── postcss@5.2.18 
│ │ ├─┬ postcss-unique-selectors@2.0.2
│ │ │ └── postcss@5.2.18 
│ │ └─┬ postcss-zindex@2.2.0
│ │   └── postcss@5.2.18 
│ └── postcss@6.0.23 
├─┬ postcss-extend@1.0.5
│ └── postcss@5.2.18 
├─┬ postcss-import@12.0.0
│ └── postcss@7.0.1  deduped
├─┬ postcss-mixins@6.2.0
│ ├── postcss@6.0.23 
│ ├─┬ postcss-js@1.0.1
│ │ └── postcss@6.0.23 
│ ├─┬ postcss-simple-vars@4.1.0
│ │ └── postcss@6.0.23  deduped
│ └─┬ sugarss@1.0.1
│   └── postcss@6.0.23 
├─┬ postcss-preset-env@5.2.3
│ ├─┬ autoprefixer@8.6.5
│ │ └── postcss@6.0.23  deduped
│ ├── postcss@6.0.23 
│ ├─┬ postcss-attribute-case-insensitive@3.0.1
│ │ └── postcss@6.0.23 
│ ├─┬ postcss-color-functional-notation@1.0.2
│ │ └── postcss@6.0.23 
│ ├─┬ postcss-color-hex-alpha@3.0.0
│ │ └── postcss@6.0.23 
│ ├─┬ postcss-color-mod-function@2.4.3
│ │ └── postcss@6.0.23 
│ ├─┬ postcss-color-rebeccapurple@3.1.0
│ │ └── postcss@6.0.23 
│ ├─┬ postcss-custom-media@6.0.0
│ │ └── postcss@6.0.23 
│ ├─┬ postcss-custom-properties@7.0.0
│ │ └── postcss@6.0.23 
│ ├─┬ postcss-custom-selectors@4.0.1
│ │ └── postcss@6.0.23 
│ ├─┬ postcss-dir-pseudo-class@4.0.0
│ │ └── postcss@6.0.23 
│ ├─┬ postcss-env-function@1.0.0
│ │ └── postcss@6.0.23 
│ ├─┬ postcss-focus-visible@3.0.0
│ │ └── postcss@6.0.23 
│ ├─┬ postcss-focus-within@2.0.0
│ │ └── postcss@6.0.23 
│ ├─┬ postcss-font-family-system-ui@3.0.0
│ │ └── postcss@6.0.23 
│ ├─┬ postcss-font-variant@3.0.0
│ │ └── postcss@6.0.23 
│ ├─┬ postcss-gap-properties@1.0.0
│ │ └── postcss@6.0.23 
│ ├─┬ postcss-image-set-function@2.0.0
│ │ └── postcss@6.0.23 
│ ├─┬ postcss-initial@2.0.0
│ │ └── postcss@6.0.23 
│ ├─┬ postcss-lab-function@1.0.1
│ │ └── postcss@6.0.23 
│ ├─┬ postcss-logical@1.1.1
│ │ └── postcss@6.0.23 
│ ├─┬ postcss-media-minmax@3.0.0
│ │ └── postcss@6.0.23 
│ ├─┬ postcss-nesting@6.0.0
│ │ └── postcss@6.0.23 
│ ├─┬ postcss-overflow-shorthand@1.0.1
│ │ └── postcss@6.0.23 
│ ├─┬ postcss-page-break@1.0.0
│ │ └── postcss@6.0.23 
│ ├─┬ postcss-place@3.0.1
│ │ └── postcss@6.0.23 
│ ├─┬ postcss-pseudo-class-any-link@5.0.0
│ │ └── postcss@6.0.23 
│ ├─┬ postcss-replace-overflow-wrap@2.0.0
│ │ └── postcss@6.0.23 
│ ├─┬ postcss-selector-matches@3.0.1
│ │ └── postcss@6.0.23 
│ └─┬ postcss-selector-not@3.0.1
│   └── postcss@6.0.23 
├─┬ postcss-reporter@6.0.0
│ └── postcss@7.0.2 
└─┬ postcss-sprites@4.2.1
  └── postcss@5.2.18 

So, currently my setup uses 4 different versions of postcss (5.2.18, 6.0.23, 7.0.1, 7.0.2) and it is not ok :) Also I receive warning from one of postcss versions:

Without `from` option PostCSS could generate wrong source map and will not find Browserslist config. Set it to CSS file path or to `undefined` to prevent this warning.

However I specify filename inside postcss-cli:

postcss --config tools/postcss.config css/*.pcss --dir css --ext css

@ai
Copy link

ai commented Aug 19, 2018

@stalniy you should ask postcss-critical-css to update cssnano, it will reduce PostCSS versions from 3 to 2.

Most of the l plugins will move to PostCSS 7 soon. It will reduce from 2 to 1.

@ai
Copy link

ai commented Aug 19, 2018

The from warning is a bad thing here. Somebody forget to set from option. I don't think it is postcss-cli, try to disable plugins one by one, to find the source.

@jonathantneal
Copy link
Collaborator

I was on vacation and missed this conversation. I was just asking something similar JLHwung/postcss-font-family-system-ui#62 (comment) so I wonder if we can resolve everything here and I’ll write up a little document explaining why things work the way they do.

@stalniy
Copy link
Author

stalniy commented Aug 21, 2018

I’ll add this article also here.

Another article explaining why and when it’s important to use peer deps https://lexi-lambda.github.io/blog/2016/08/24/understanding-the-npm-dependency-model/

It has a good example with React which shows the difference between deps and peer deps. And explains why React needs to be specified in peer deps

@stalniy
Copy link
Author

stalniy commented Aug 21, 2018

As an example you can also check Babel plugins. The idea is completely the same and all Babel plugins specify Babel/core package in peer deps

@halfzebra
Copy link
Contributor

I do agree that there is a problem with the organization of plugins.

Choosing peerDependencies over direct dependencies introduces a tradeoff of reliability over the ease of maintenance. As a package maintainer, I would think twice before sacrificing user experience.

Because there's no official curated set of plugins for PostCSS, it's hard to justify any significant changes to the conventional setup.

Here is what @ai says about plugins under PostCSS organization(I don't mean to harm the image of the organization or the quality of the plugins):

They are not official. They just made by people who speak often together.

Source: postcss/postcss#1173

Please read here https://lexi-lambda.github.io/blog/2016/08/24/understanding-the-npm-dependency-model/ for details

I don't think it's entirely fair to consider React as an ultimate example, because it's a client-side library, where code duplication has performance cost(longer loading time and evaluation time). I'm playing the devil's advocate here, but it's not the same as an npm install.

Babel did not specify peerDependencies before version 7, and they version all the packages with the same version number. This is possible because of the monorepo they are running, and thanks to that users don't have to figure out which version of @babel/core to install.

@stalniy
Copy link
Author

stalniy commented Aug 21, 2018

@halfzebra I agree with you about babel. Until all plugins in one hands and versions are up to date, I don't care how they are managed :)

@pascalduez
Copy link

pascalduez commented Aug 21, 2018

Pasting a relevant reply from @ai
zgreen/postcss-critical-css#28 (comment)

There are 2 problems with peerDependencies in case of PostCSS plugin:

  • We use PostCSS dependency to detect what AST API plugin use. It will be broken on peerDependencies.
  • Often requires non-obvious actions from user.

PostCSS release major version only once in several years. It is easier to update dependencies.

I'm quite curious about this one:

We use PostCSS dependency to detect what AST API plugin use. It will be broken on peerDependencies.

@stalniy
Copy link
Author

stalniy commented Aug 21, 2018

Also to move some info into the main thread:

Currently I have 97 instances of postcss in my node_modules

find node_modules/ -name postcss -type d | wc -l
97

$ npm -v
6.1.0
$ node -v
v10.5.0

My package.json:

{
  "name": "ruba4ok",
  "version": "1.0.0",
  "description": "skin for ruba4ok.com",
  "author": "Sergii Stotskyi",
  "license": "ISC",
  "engines": {
    "node": ">=6.0.0"
  },
  "scripts": {
    "compile.css": "postcss --config tools/postcss.config ./${SKIN:-default}/css/*.pcss --dir ./${SKIN:-default}/css --ext css",
    "minify.js": "find ../../../../media/js -name *.js ! -name *.min.js -exec bash -c 'uglifyjs \"$1\" -c -m --keep-fnames -r \\$super --screw-ie8 --source-map \"${1/.js/.min.js}.map\" -o \"${1/.js/.min.js}\"' _ {} \\;"
  },
  "devDependencies": {
    "cssnano": "^4.0.5",
    "postcss-cli": "^6.0.0",
    "postcss-critical-css": "^3.0.1",
    "postcss-extend": "^1.0.5",
    "postcss-import": "^12.0.0",
    "postcss-mixins": "^6.2.0",
    "postcss-preset-env": "^5.2.3",
    "postcss-reporter": "^6.0.0",
    "postcss-sprites": "^4.2.1",
    "uglify-js": "^2.8.22"
  },
  "dependencies": {
    "nouislider": "^11.1.0"
  }
}

@ai
Copy link

ai commented Aug 21, 2018

All this files are just links to the same files, so it doesn't take too much space.

@pascalduez
Copy link

Humm running npm list postcss on a project shows an crazy amount of postcss packages (not links), each plugin having its own node_modules with it. There a mix of PostCSS 5, 6, 7.
So for some reasons it's not deduped properly.

@ai
Copy link

ai commented Aug 21, 2018

@pascalduez yarn and npm doesn't make a copy. It link files if the directory contain the same version.

BTW, you should open a issue for every plugin which use PostCSS 5, since they are not compatible with PostCSS 7.

@stalniy
Copy link
Author

stalniy commented Aug 21, 2018

@ai these are not links. this is a proof:

find node_modules/ -name postcss -type d -exec stat -c '%i' '{}' \; | uniq | wc -l
97

If they were links we would get 4 unique inodes

@stalniy
Copy link
Author

stalniy commented Aug 21, 2018

they are also not symlinks because for symlinks there is a separate -type l option which shows only links

@pascalduez
Copy link

pascalduez commented Aug 21, 2018

yarn and npm doesn't make a copy. It link files if the directory contain the same version.

They are not links (as symlinks) but real folders with all the files in.

you should open a issue for every plugin which use PostCSS 5, since they are not compatible with PostCSS 7.

I will, but the after having done that on several plugins for PostCSS 7, most of them are not yet published, so without the rights on the repo and npm it seems a bit useless.

@ai
Copy link

ai commented Aug 21, 2018

I am talking about files, not a directories

@stalniy
Copy link
Author

stalniy commented Aug 21, 2018

@ai do you mean that npm/yarn links individual files instead of folder? This sounds at least strange because would require more resources to utilize and I don't see the purpose. I will check this

@ai
Copy link

ai commented Aug 21, 2018

@stalniy making a link is much faster than copy a file

@ai
Copy link

ai commented Aug 21, 2018

Don't miss software link (special file type with path to different file) and hardware link (multiple files are pointing to the same memory)

@stalniy
Copy link
Author

stalniy commented Aug 21, 2018

@ai can you provide proofs about what you say?

@stalniy
Copy link
Author

stalniy commented Aug 21, 2018

there are only 2 types of links on linux systems: https://www.linuxtopia.org/online_books/introduction_to_linux/linux_Link_types.html

And again links which are not symlinks must have the same inode and this is not the case for postcss:

As you can see in this script I retrieve inodes of all js files. uniq ensures that there are no duplicates in output and wc -l counts lines

for path in `find node_modules/ -name postcss -type d`; do 
  find $path -type f -name \*.js -exec stat -c '%i' '{}' \;
done | uniq | wc -l
2512

The result is 2512 unique inodes.

@ai
Copy link

ai commented Aug 21, 2018

@stalniy not today (I need to fix few issues in Autoprefixer and release new Browserslist).

Fast fact: yarn writes "linking" and not "coping" in terminal during installation.

@stalniy
Copy link
Author

stalniy commented Aug 21, 2018

$ ln package.json package-link.json
$ stat -c '%i' package-link.json
15206458
$ stat -c '%i' package.json
15206458

As you can see these are hard links which have the same inode

@stalniy
Copy link
Author

stalniy commented Aug 21, 2018

@ai yarn does work a bit better but it shows 34 duplicates of postcss

$ find node_modules/ -name postcss -type d  | wc -l
34
$ yarn --version
1.9.4

The same tests show the same results, there are duplicates:

# test whether folders are links
$ find node_modules/ -name postcss -type d -exec stat -c '%i' '{}' \; | uniq | wc -l
34

# test whether all files are links
$ for path in `find node_modules/ -name postcss -type d`; do    find $path -type f -name \*.js -exec stat -c '%i' '{}' \;; done | uniq | wc -l
869

@jonathantneal
Copy link
Collaborator

I’m sticking with the pattern established in other plugins unless/until @ai wants to make this change across the board. PRs or new issues are welcome, but I’m closing this due to inactivity.

@stalniy
Copy link
Author

stalniy commented Sep 18, 2018

@ai any updates?

@jonathantneal I don’t think this is the good way to communicate issues with community. There is a problem. I provided proves. The fact that you close this issues due to inactivity makes me think that the problem just ignored by the post css creators.

Another point. Everybody “redirects” decision making to @ai what makes me think that if @ai doesn’t respond as in this case (or just stop working on post css), all plugins will be abandoned and the project will die.

It’s pity...

@jonathantneal
Copy link
Collaborator

@stalniy, I’m still open to considering a change, but I would need clearer examples of prior art and the changes requested of various plugins before I made a decision.

What I do feel strongly about is closing this thread. A new thread could be made, possibly on the postcss project that is more succinct. This thread sat vacant for a month. There are some robots that do this automatically and lock the thread, and I understand why that can be valuable, but I’m trying to be a little more human about it, at least until I get burned, I suppose.

@ai
Copy link

ai commented Sep 18, 2018

It is just better to update all plugins to PostCSS 7.

@a-x-
Copy link

a-x- commented May 24, 2019

any updates?
why postcss is not peerDep here yet? :-)

@stalniy
Copy link
Author

stalniy commented Feb 17, 2020

@ai from angular docs about creating angular specific libs (i.e., angular plugins), what clearly maps to postcss plugins:

Angular libraries should list all @angular/* dependencies as peer dependencies. This ensures that when modules ask for Angular, they all get the exact same module. If a library lists @angular/core in dependencies instead of peerDependencies, it might get a different Angular module instead, which would cause your application to break.

@ai
Copy link

ai commented Feb 17, 2020

I can think about changing our policy in PostCSS 8.0, but 8.0 release funding is not very successful.

@stalniy
Copy link
Author

stalniy commented Feb 17, 2020

I guess that majority of people still use SCSS, a lot of libraries use SCSS. Some may not even know which power postcss can bring to the project.

I also in majority of commercial projects use only autoprefixer and that's it.
Frankly speaking, it was a big pain to use postcss with its dependency issues... Eventually I decreased amount of plugins in order to get what I want. But I believe it will improve some day.

So, Good luck!

@ai
Copy link

ai commented Feb 17, 2020

I guess that majority of people still use SCSS

No, PostCSS has 4 times more downloads than Sass.

People use Sass only as a preprocessor, but PostCSS is used as preprocessor, linter, minifier, and parser.

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

No branches or pull requests

6 participants