Support global config with load config package #97

ertrzyiks · 2016-09-14T15:00:31Z

It's going to address #88 , but it may require more attention in the early phase. Any feedback is welcome.

ai · 2016-09-14T18:02:35Z

Great! Fast work :). Sorry, I could review it only tomorrow (I am not so fast :( )

ertrzyiks · 2016-09-14T19:55:47Z

Awesome! Please keep in mind that I've just started and submitted PR already because it will not be one liner and any tip on this stage can prevent a headache later on :)

ai · 2016-09-15T05:37:28Z

Looks good. Only few very small things:

Why you comment postcssrc?
Could you please fix my weird manually indent in package.json :D

ertrzyiks · 2016-09-15T06:46:27Z

In the middle of work my gulp process was dying without any notice. It was fine when I commented out that part. It may even work right now, a lot of code changed since then. I will check it today.
Ok, I will revert all unrelated changes there, will leave only a new dependency entry.

From what I see it may be tricky to support pack feature when config loader is used. Are you ok with having it only for the current behavior?

ai · 2016-09-15T06:47:41Z

Feel free to return error on using pack with config

ertrzyiks

Still some things to improve

ertrzyiks · 2016-09-19T14:02:27Z

@ai I think it's done more or less. I've tried to utilize postcss-load-config better, but ended up using it only when there is no explicit postcss config for webpack.

I've added more tests and they require separate configs, so we have multiple gulp tasks generated per each config scenario.

What do you think about it?

ertrzyiks

Self-review done, ready for the final review

ai · 2016-09-19T14:45:26Z

Great work, I will review it tomorrow (sorry, working day was started)

ai · 2016-09-21T02:34:57Z

Looks good. Sorry, it could take a while (need be sure, that it will help in webpack 2 too)

ai · 2016-09-23T10:00:35Z

New webpack 2 issue bring new question. Could we do everything with this config?

How we could cover addDependecyTo case in this PostCSS global config? https://github.com/postcss/postcss-loader#with-postcss-import

/cc @michael-ciniawsky

ertrzyiks · 2016-09-23T10:31:55Z

I don't think so we can cover addDependecyTo in the current version of this config, we have no access to that webpack reference when defining configuration. Even with webpack 1 we can not support pack feature, so I would see load-config solution as a secondary right now.

ai · 2016-09-23T11:06:03Z

@ertrzyiks do we have any way to pass webpack instance to config?

If not, we could ask @michael-ciniawsky for some solution

ertrzyiks · 2016-09-23T12:41:43Z

@ai we could do this but by breaking the whole idea of having config object with primitive values (as it requires a config value being a function).

Maybe @michael-ciniawsky can come up with some idea. I'm wondering if it would be possible to feed postcss with webpack instance and use internal pub-sub communication (if any) between postcss and plugins to propagate it.

ai · 2016-09-23T12:48:49Z

Hm. Maybe we could use result.message and set new files to some dependencies. And then it will work out of box in postcss-loader?

@ertrzyiks sorry for delay, it is very big API changes, let’s think twice to avoid haters to hate :D

ai · 2016-09-23T12:51:20Z

@swernerx @MoOx @TrySound @borodean what do you think if all import plugin should set absolute path for all used resources to use it then in webpack?

result.messages.push({ type: 'dependency', path: '/home/ai/test/images/logo.png' })

As result hot reload will work out-of-box in postcss-loader, without painful config.

borodean · 2016-09-23T12:56:30Z

@ai that should be fine for postcss-assets, however, I'd need someone's help implementing this.

swernerx · 2016-09-23T17:06:27Z

@ai IMHO this seems to be a good idea. Makes it far easier to track paths from different "viewpoints" and unify them.

MoOx · 2016-09-23T20:36:44Z

Since import plugins are used first, there is no need to put all resources, because the work will be done by webpack when css is inlined by the plugin. So maybe ok for the path, but no need for all resources. Anyway, for postcss-import, there is a total mess to handle. @TrySound made some huge changes and never really finished the job, so there are tons of issues related to race conditions... But current algorithm looks to complicated for my brain, and I am busy with new projects, so no time to handle this... That's why I called for contributor for postcss-import, but, as usual, not a single person responded.

ai · 2016-10-07T18:28:21Z

Finally merged. Thanks, great work. I just need to update docs, add result.messages with dependencies and we are ready for release.

michael-ciniawsky · 2016-10-07T21:57:36Z

@ai docs WIP :)

Sponsor badge directly under the logo or is the sponors section at the bottom ok aswell?

postcss-load-config supports passing a function now, finishing the eval stuff and test it with especially this loader, if it does what it is supposed to be :). @ai can you give me an example/explanation ( little more in-depth :) ) of how the result.messages.push(dependencies) will look like?

ai · 2016-10-07T22:02:31Z

@michael-ciniawsky great work! Few moments:

Yeap, sponsor badge should be right after main description.
You could remove my name from README :).
I love your style. But could we keep old, please? =^_^= I love that right now all PostCSS project has same style. Maybe in the future I will be ready for global changes.
We also added plugins loader query params for webpack 2.0 Allow options via query. #104

I could pull your changes and fix all this list tomorrow (you did a lot with config loader) :).

ai · 2016-10-07T22:09:35Z

Hm, by the way, maybe somebody want to take maintaining for this loader? It is not hard.

But I ma not webpack user at all. You showed great productivity and good understanding of webpack.

I think project will be better if maintainer could at least test it on work project :D.

michael-ciniawsky · 2016-10-07T22:51:54Z

@ai

kk
you mean change to @ai, or completely, guessing the latter 😛
yep of course, didn't know there was already consitent styleing accress postcss org,

Maybe in the future I will be ready for global changes.

I can do and would liveto do it, but secondary chore work :)

gotcha i will add it.

I can do it, already maintaining html-loader, posthtml-loader. webpack org peasant member :D
Maybe @ertrzyiks wants to give a hand too, we can share the 'burden'

Send invite and grant access here, I can also transfer the load-* stuff then and update it at the weekend, to finally move out of alpha, beta. It makes things a lot easier for me (update it to v1.0.0 asap, add jsdoc, keep track of webpack 2 beta etc.)

michael-ciniawsky · 2016-10-07T22:59:54Z

👍

But transfering the postcss-load-* repos doesn't work, should the stay at my github?

ai · 2016-10-07T23:03:58Z

@michael-ciniawsky yeap, you could keep it :)

If you want to move them to postcss org, I could give you a access to org too.

michael-ciniawsky · 2016-10-07T23:11:20Z

@ai Can some the content related ajustments stay?

yeap, you could keep it :)

kk

ai · 2016-10-07T23:14:41Z

@michael-ciniawsky what exactly “content related ajustments”?

I like that you split webpack 1 and webpack 2. But maybe we should promote your config first?

Install
Usage
1. PostCSS Config
2. Webpack 2 config
3. Webpack 3 config
Options
Examples

michael-ciniawsky · 2016-10-07T23:30:24Z

@ai README

The structure is awesome 👍
I had the postcss.config.js example when postcss-load-config is finished + #104

ai · 2016-10-08T06:00:36Z

@michael-ciniawsky I pulled docs changes, thanks.

michael-ciniawsky · 2016-10-08T06:13:45Z

@ai so you want to push them later? I'm asking because otherwise I will do it today, during the day with postcss-load-config release? :) It's neglectible, adding the postcss.config.js example later, just don't want to mess things up here

ai · 2016-10-08T06:14:42Z

I wrk polish docs today 😊👍

michael-ciniawsky · 2016-10-08T06:17:54Z

👍

ai · 2016-10-08T08:20:26Z

I finished all changes and ready for release. Could somebody test this plugin on real application? (Just replace version in package.json to postcss/postcss-loader)

richardscarrott · 2016-10-08T11:05:52Z

@ai I get a webpack validation error when using the following config:

{
        loader: 'postcss-loader',
        plugins: () => [require('autoprefixer')]
}

I think Webpack 2 expects loaders to nest options like this, e.g.

{
        loader: 'postcss-loader',
        options: {
            plugins: () => [require('autoprefixer')]
        }
}

webpack: 2.1.0-beta.25
postcss-loader: postcss/postcss-loader (d867d02)

michael-ciniawsky · 2016-10-08T11:19:44Z

@richardscarrott yep, i was wondering about that too, your example with

{
  loader: 'postcss-loader'
  options: {
    plugins: () => [ require('autoprefixer') ]
  }
}

is the correct one, but to my knowledge, it's currently not possible to use a function inside options. This is a work in progress and currently 'polyfilled' by the LoaderOptionsPlugin

const { LoaderOptionsPlugin } = require('webpack')

const config = {
  module: {
   rules: [
    {
     test: /\.css$/,
     use: ['style-loader', {loader: 'css-loader', options: { importLoaders: 1 }}, 'postcss-loader']
    }
  ],
  plugins : [
    new LoaderOptionsPlugin({
      options: {
        postcss: () => {
          return [ require('autoprefixer') ]
       }
     }
   })
 ]
}

module.exports = config

I check the webpack slack quickly, just one moment please :)

ai · 2016-10-08T11:33:12Z

@richardscarrott @michael-ciniawsky fix for webpack 2 config example: e6351b8

Is it correct?

richardscarrott · 2016-10-08T11:43:41Z

@michael-ciniawsky yeah I just noticed it does work with options.plugins as a function but as soon as you try to use the ExtractText plugin it fails because that serializes the query / options which obviously won't work with a fn.

Looks like your LoaderOptionsPlugin workaround is the best approach or I guess create a separate postcss.config.js.

@ai I can't find that commit?

michael-ciniawsky · 2016-10-08T11:50:09Z

@richardscarrott kk, I will check that
@ai can't find that commit too 😛, by the way an example with ExtractTextPlugin maybe valuable in general :)

ai · 2016-10-08T11:52:59Z

Oops, it was merge confict :D. Fixed.

michael-ciniawsky · 2016-10-09T04:43:13Z

@ai basically yes, currently nope 😛 , and combined with ExtractTextPlugin for now LoaderOptionsPlugin is definitely needed, I add an example for usage with ExtractTextPlugin, but let me finished all the postcss-load-config stuff first :)

{
   loader: 'postcss-loader'
   options: { 
     plugins: () => [...plugins] // Error
   } 
}

!==

{
   loader: 'postcss-loader'
   options: () => { parser: sugarss, plugins: [...plugins] } 
   // According to @sokra this should be possible since webpack v2.1.0-beta.25, 
   // when ['loader-utils'].parseQuery(loader.query) is used as it currently is in postcss-loader
   // but API Schema complains that typeof module.rules[1].use[2].options === 'string' || 'options'
   // always fun, fun... , fun...... :D  
}

But there seems to be an issue with parseOptions in postcss-loader in general

webpack.config.js
1.

{
   loader: 'postcss-loader',
   options: {
     plugins: () => [ require('postcss-nested') ]
   }
}

postcss-loader/index.js

var params = loaderUtils.parseQuery(loader.query)

console.log(params)
// => {} params.plugins stripped

2.

{
   loader: 'postcss-loader',
   options: {
     parser: 'sugarss',
     plugins: () => [ require('postcss-nested') ]
   }
}

postcss-loader/index.js

var params = loaderUtils.parseQuery(loader.query)

console.log(params)
// => { parser: 'sugarss' } params.plugins stripped

3.

{
   loader: 'postcss-loader',
   options: {
     parser: 'sugarss',
     plugins: [ require('postcss-nested') ]
   }
}

postcss-loader/index.js

var params = loaderUtils.parseQuery(loader.query)

console.log(params)
// => { parser: 'sugarss' plugins: [ null ] } params.plugins not loaded

ai · 2016-10-09T11:16:05Z

@MoOx @michael-ciniawsky so in practice webpack anyway strip functions from options?

So, should we remove plugins from query options and force to use postcss.config.js?

MoOx · 2016-10-09T12:56:35Z

Webpack 2 remove this restriction. You can pass anything in query now.

ai · 2016-10-09T12:59:00Z

@mood it is great. But @michael-ciniawsky shows different 😕. Do we have any mistakes? Maybe we should replace loaderUtils.parseQuery to accepts functions?

ai · 2016-10-09T15:15:00Z

I made a test case: https://github.com/ai/webpack2-postcss-test

plugins key works perfect from webpack config, but postcss config has same issue. I will open a issue in postcss-load-config.

MoOx · 2016-10-09T17:07:00Z

It's written in https://github.com/webpack/webpack/releases/tag/v2.1.0-beta.23
If this does not work, you are doing something wrong.

ertrzyiks force-pushed the support-global-config-with-load-config-package branch 2 times, most recently from 5e4f709 to 71943ce Compare September 15, 2016 10:10

ertrzyiks commented Sep 15, 2016

View reviewed changes

ertrzyiks force-pushed the support-global-config-with-load-config-package branch 6 times, most recently from 9a90364 to 29e279f Compare September 19, 2016 13:56

ertrzyiks commented Sep 19, 2016

View reviewed changes

ai mentioned this pull request Sep 20, 2016

Incompatible with Webpack 2.1.0-beta.23 configuration validation #99

Closed

Support global config with load config package

93b7003

ertrzyiks force-pushed the support-global-config-with-load-config-package branch from 29e279f to 93b7003 Compare September 22, 2016 08:11

ai merged commit 93b7003 into webpack-contrib:master Oct 7, 2016

ai mentioned this pull request Oct 9, 2016

Doesn't compiles css #106

Closed

ertrzyiks deleted the support-global-config-with-load-config-package branch October 9, 2016 16:51

Support global config with load config package #97

Support global config with load config package #97

Conversation

ertrzyiks commented Sep 14, 2016 • edited Loading

ai commented Sep 14, 2016

ertrzyiks commented Sep 14, 2016

ai commented Sep 15, 2016

ertrzyiks commented Sep 15, 2016

ai commented Sep 15, 2016

ertrzyiks left a comment

Choose a reason for hiding this comment

ertrzyiks commented Sep 19, 2016 • edited Loading

ertrzyiks left a comment

Choose a reason for hiding this comment

ai commented Sep 19, 2016

ai commented Sep 21, 2016

ai commented Sep 23, 2016

ertrzyiks commented Sep 23, 2016

ai commented Sep 23, 2016

ertrzyiks commented Sep 23, 2016 • edited Loading

ai commented Sep 23, 2016 • edited Loading

ai commented Sep 23, 2016

borodean commented Sep 23, 2016

swernerx commented Sep 23, 2016

MoOx commented Sep 23, 2016

ai commented Oct 7, 2016

michael-ciniawsky commented Oct 7, 2016

ai commented Oct 7, 2016

ai commented Oct 7, 2016

michael-ciniawsky commented Oct 7, 2016 • edited Loading

michael-ciniawsky commented Oct 7, 2016

ai commented Oct 7, 2016 • edited Loading

michael-ciniawsky commented Oct 7, 2016

ai commented Oct 7, 2016

michael-ciniawsky commented Oct 7, 2016 • edited Loading

ai commented Oct 8, 2016

michael-ciniawsky commented Oct 8, 2016

ai commented Oct 8, 2016

michael-ciniawsky commented Oct 8, 2016

ai commented Oct 8, 2016 • edited Loading

richardscarrott commented Oct 8, 2016 • edited Loading

michael-ciniawsky commented Oct 8, 2016 • edited Loading

ai commented Oct 8, 2016

richardscarrott commented Oct 8, 2016 • edited Loading

michael-ciniawsky commented Oct 8, 2016 • edited Loading

ai commented Oct 8, 2016

michael-ciniawsky commented Oct 9, 2016 • edited Loading

ai commented Oct 9, 2016

MoOx commented Oct 9, 2016

ai commented Oct 9, 2016

ai commented Oct 9, 2016

MoOx commented Oct 9, 2016

ertrzyiks commented Sep 14, 2016 •

edited

Loading

ertrzyiks commented Sep 19, 2016 •

edited

Loading

ertrzyiks commented Sep 23, 2016 •

edited

Loading

ai commented Sep 23, 2016 •

edited

Loading

michael-ciniawsky commented Oct 7, 2016 •

edited

Loading

ai commented Oct 7, 2016 •

edited

Loading

michael-ciniawsky commented Oct 7, 2016 •

edited

Loading

ai commented Oct 8, 2016 •

edited

Loading

richardscarrott commented Oct 8, 2016 •

edited

Loading

michael-ciniawsky commented Oct 8, 2016 •

edited

Loading

richardscarrott commented Oct 8, 2016 •

edited

Loading

michael-ciniawsky commented Oct 8, 2016 •

edited

Loading

michael-ciniawsky commented Oct 9, 2016 •

edited

Loading