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

other plugin invoke twice #70

Closed
sunnylost opened this issue Aug 21, 2017 · 9 comments · Fixed by #111
Closed

other plugin invoke twice #70

sunnylost opened this issue Aug 21, 2017 · 9 comments · Fixed by #111

Comments

@sunnylost
Copy link

const postcss = require( 'postcss' )
const modules = require( 'postcss-modules' )

let testPlugin = postcss.plugin( 'test-plugin-1', () => {
    return () => {
        console.log( 'test-plugin-1 running' )
    }
} )

let testPlugin2 = postcss.plugin( 'test-plugin-2', () => {
    return () => {
        console.log( 'test-plugin-2 running' )
    }
} )

postcss( [
    modules( {
        getJSON( _, json ) {
        }
    } ),
    testPlugin(),
    testPlugin2()
] )
    .process( `
    .title {
        color: green;
    }` )
    .then( () => {
    } )

if you run this code, that two plugins were running twice. Did i use this plugin in a wrong way?

@EECOLOR
Copy link

EECOLOR commented Sep 19, 2017

No you did not. I don't know why, but this plugin executes postcss with all other plugins: https://github.com/css-modules/postcss-modules/blob/master/src/index.js#L75

The simple fix is:

const plugins = [
  require('postcss-plugin-composition', [
      // plugins that need to run on each individual file (import, modules, etc.)
  ]),
  // plugins that need to run on the result (once)
]

Please be aware of: btd/postcss-plugin-composition#1

A copy without this problem:

const postcss = require('postcss')

module.exports = postcss.plugin('postcss-plugin-composition', plugins => {
  if(!Array.isArray(plugins)) throw new Error('`options` for postcss-plugin-composition must be array of plugins')

  return (root, { opts, messages }) => 
    postcss(plugins)
      .process(root, opts)
      .then(({ messages: m }) => { messages.push.apply(messages, m) })
})

@EECOLOR
Copy link

EECOLOR commented Sep 19, 2017

@maintainer / @developer / @owner, the 'double' execution of plugins is in this plugin since the initial commit. Could you please evaluate if it is still required to run postcss with all plugins again?

@gzzhanghao
Copy link

It was introduced in 78b1c40#diff-1fdf421c05c1140f6d71444ea2b27638 but I have no idea about what it solved 😕

@peter-mouland
Copy link

is there a solution for this which does not involve the composition plugin?

@EECOLOR
Copy link

EECOLOR commented Feb 16, 2019

@peter-mouland I recommend you move away from this module as it is not properly maintained. I have opened several pull requests and issues (to upgrade the postcss dependency) in October, but they are still open.

Both our our webpack css loader and the defaul css loader have adopted another strategy of supporting css modules.

Incidentally, this other approach does not involve the composition plugin.

@madyankin
Copy link
Owner

madyankin commented Feb 18, 2019

@EECOLOR actually, there are no opened PRs, as you can see. But if you'd like to maintain the project, I can add you to the project.

It's always easier to say that something is not maintained or request something from a maintainer than provide some actual help to people who make tools you use every day.

I don't use PostCSS or CSS now, I even don't do web development. So, any real help with the package is highly appreciated.

@peter-mouland
Copy link

to clarify my position, and maybe help others who find this. After trying to create a minimal repo with the bug, I found that output was duplicated even without this plugin. I was using postcss-preset-env and the output would be

color: #fff;
color: #fff;
color: var(--white);

adding preserve: false got me to :

color: #fff;

not sure if that is the correct fix, but it's fine for me for now.

@EECOLOR
Copy link

EECOLOR commented Feb 19, 2019

It's always easier to say that something is not maintained or request something from a maintainer than provide some actual help to people who make tools you use every day.

I agree with you completely and not just saying it, I have done my fair share.

there are no opened PRs

@Outpunk, I could not yet add a pull request for this repository as it's dependencies first needed to be updated (note that this pull request also refers to 4 other pull requests): css-modules/css-modules-loader-core#231

But if you'd like to maintain the project, I can add you to the project.

Just like you I don't write CSS, I however have never written CSS, I would be the worst person for the job.

On top of that, the extra layers of indirection indirection this module introduces is not something I would advertise. I would archive this (and css modules-loader-core) in favor of the elemental post css plugins.

For people finding this repository I would point to implementations of css modules and probably just add an example postcss config in the README.

@madyankin
Copy link
Owner

On top of that, the extra layers of indirection indirection this module introduces is not something I would advertise. I would archive this (and css modules-loader-core) in favor of the elemental post css plugins.

You're definitely right, I'm totally agree with this point. Maybe, I should consider deprecating the plugin in favor of more simple tools or completely rewrite it someday.

madyankin pushed a commit that referenced this issue Aug 20, 2020
* only apply user plugins to loader

The input `css` has already been processed by any plugins before `postcss-module`, and the output will be processed by any plugins after. We shoud not run all the user plugins again.

However, for anything that is loaded in by the loader (such as from `composes`), we should run the plugins that were listed before `postcss-modules`, so that the css that is brought in can catch up.

Fixes  #70

* update tests

* also test plugin isn't called multiple times

* include filename in comment

* add start and end markers

* use findIndex

* separate tests for only calling plugins once

* fix missing check
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.

5 participants