Skip to content
This repository has been archived by the owner on Mar 6, 2024. It is now read-only.

PostCSS plugins are ignored #84

Closed
ertrzyiks opened this issue Sep 13, 2016 · 18 comments
Closed

PostCSS plugins are ignored #84

ertrzyiks opened this issue Sep 13, 2016 · 18 comments

Comments

@ertrzyiks
Copy link

ertrzyiks commented Sep 13, 2016

Hey, I've a problem with postcss-loader and have created the smallest repo to reproduce it.

https://github.com/ertrzyiks/happypack-postcss-example

First of all I've found that not all options defined in my config are passed to the loader, so updated SERIALIZABLE_OPTIONS array with my key, but it still doesnt work. Looks like PostCSS plugins are ignored, because they are passed as an array of functions and can not be serialized here in launchAndConfigureThreads.

Any clues how to resolve it?

@amireh
Copy link
Owner

amireh commented Sep 13, 2016

How about we give you a chance to deserialize custom options in such a way that you can reproduce this list of functions on the background threads?

The API I'm thinking of is something like this:

{
  postcss__happy: [
    {
      path: 'autoprefixer',
      options: {
        browsers: ['ie 10']
      }
    }
  ],

  plugins: [
    new HappyPack({
      customOptionSerializer: function(compilerOptions) {
        return { postcss: compilerOptions.postcss__happy };
      },

      customOptionDeserializer: function(rawOptions) {
        return {
          postcss: function() {
            return rawOptions.postcss.map(function(postcssModule) {
              return require(postcssModule.path)(postcssModule.options);
            });
          }
        };
      }
    })
  ]
}

This will work with the only condition that customOptionDeserializer does not depend on the closure (i.e. it can be serialized using toString()). And the reason we use a different option name instead of postcss so as to not conflict with the original option in case you're not using happypack for a certain build.


Honestly, I know this isn't a pretty solution but it's the best I can think of in terms of solving your use-case and not causing regressions. There are good reasons why we don't try to serialize function options.

It might be worth noting that this could've been solved more reasily on the loader's end since if they were accepting paths to functions and arguments (like [{ path: 'autoprefixer', options: { browsers: [ 'ie10' ] } }]) and then materialize them into a list of functions then we could've serialized postcss without any modifications.

If you like the suggested solution or can think of a different approach, let me know.

@ertrzyiks
Copy link
Author

Thanks for the fast reply @amireh

Your suggestion looks fine and might be enough for most use cases, but it will not help me. I mean, it would increase a complexity of my code to the level I think is not worth implementing. It's good enough to know I did not miss something obvious and it is actually a problem in 3rd party, not my usage.

I was very surprised that there is no way to configure postcss-loader or even postcss itself using a plain object. Unfortunately fixing it in the loader will be replacing one problem with another, since in the latest version it uses internal this._compilation https://github.com/postcss/postcss-loader/blob/master/index.js#L86 and crash when used with happypack.

In the end, I will try to exclude postcss from parallelization in my case and will try to fix it on the loader side, since it's way easier as you said.

Cheers!

@ertrzyiks
Copy link
Author

Looks like there is a discussion around postcss to support plain object config, see postcss/postcss#813

Meanwhile I will suggest postcss-loader to support one of helper libraries: postcss/postcss#813 (comment)

@ertrzyiks
Copy link
Author

ertrzyiks commented Oct 31, 2016

Hey @amireh, the latest postcss-loader supports alternative configuration options which work with happypack 🎉 Can we close this ticket or do we need an action point before? Like update wiki

@brandondoran
Copy link

Hi @ertrzyiks. I have been trying to get postcss-loader with autoprefixer plugin working with HappyJack but haven't had any luck. I've tried the postcss-loader config option for Webpack 1 here: https://github.com/postcss/postcss-loader/blob/1.1.0/README.md#webpack-1x-config. I've also tried with a postcss.config.js file. Would you mind sharing how you were able to get it to work?

@ertrzyiks
Copy link
Author

@brandondoran I've updated my example repo with the latest postcss-loader and you can see it working: https://github.com/ertrzyiks/happypack-postcss-example

@amireh
Copy link
Owner

amireh commented Nov 7, 2016

That's great! I'm glad to hear, and thanks a lot @ertrzyiks for providing an example. I will incorporate it into the repository or perhaps make a Wiki article for it. Which do you think is best?

(Closing since it's resolved.)

@amireh amireh closed this as completed Nov 7, 2016
@ertrzyiks
Copy link
Author

@amireh I see you have a lot of examples in the repository, feel free to incorporate my example as well. I would just add a note to the wiki to clear that postcss-loader is fully supported since v1.1.0 (master readme does not even mention problematic webpack-1 config approach anymore)

@amireh
Copy link
Owner

amireh commented Nov 7, 2016

Yeah, I think we can do a better job at reporting known issues in the docs. Thanks again, I'll look into this.

@brandondoran
Copy link

@ertrzyiks Thanks so much for the example! I've noticed that when I add ExtractTextPlugin usage I get the following warning:

WARNING in ./~/happypack/loader.js?id=sass!./client/assets/styles/account.scss
this._compilation is not available thus `postcss-loader-before-processing` is not supported

However, the css file does seem to build correctly. It looks like you had a commit to postcss-loader to emit the warning instead of crashing. Is it safe to ignore this warning?

Out of curiosity I cloned https://github.com/ertrzyiks/happypack-postcss-example and added ExtractTextPlugin and I get the warning there too.

@ertrzyiks
Copy link
Author

Hey @brandondoran I'm glad that you find it useful ) It's totally safe to ignore that warning as long as everything works for you. I've added this warning to avoid crash and leave a hint so in case of any strange problem with postcss-loader and happypack you have at least one thing to check.

@brandondoran
Copy link

Hey @ertrzyiks, awesome, that is what I figured. Thanks again for your help on this!

@acidicX
Copy link

acidicX commented Feb 17, 2017

@ertrzyiks I'm also using the ExtractTextPlugin and I'm getting those warnings. It is possible to make them less verbose? Maybe only one warning instead of 200 😉 (depending on the amount of files in my case)?

@ertrzyiks
Copy link
Author

@acidicX this warning comes from postcss-loader, not ExtractTextPlugin. It is triggered because happypack does not support this._compilation property and some loader/plugins are not fully functional. Maybe we could add an option to acknowledge lack of _compilation and explicitly silent this warning...

@creage
Copy link

creage commented Feb 24, 2017

We definitely need to suppress that warning if result is successful and is correct.

@ertrzyiks
Copy link
Author

@creage @acidicX I've created an issue in the postcss-loader repo, lets see what can be done to mitigate this problem. See webpack-contrib/postcss-loader#179

@creage
Copy link

creage commented Mar 1, 2017

Seems to be fixed with 1.3.3 release of postcss-loader. I've tested it and have no warnings any more.

@dickeylth
Copy link

So I guess the postcss-loader row in https://github.com/amireh/happypack/wiki/Loader-Compatibility-List could be updated now?

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