refactor: improved loading algorithm of options and configs #234

alexander-akait · 2017-05-16T14:25:31Z

Why?

No todo 😄
Always load config if your pass config.path, now it is not working as expected if i pass config.path and sourceMap: false my config file not loading -> options.config && !sourceMap && length > 1 (i want to load config and set sourceMap use loader options ), also sometimes i want setup some option from options loader.
Some perf (we don't create rc object because it is not necessary if i use only options without options.config)
Allow to passing options to postcss throw loader.options, now it is possibly only using postcss config.

alexander-akait · 2017-05-17T17:59:17Z

michael-ciniawsky · 2017-05-17T19:56:11Z

package.json

@@ -33,6 +33,7 @@
    "docs": "jsdoc2md lib/index.js > LOADER.md",
    "pretest": "npm run lint && npm run test:build",
    "test": "jest",
+    "test-only": "npm run test:build && jest",


Why this ? 😛

@michael-ciniawsky when i write code i want run only test to avoid regression (without lint), full test with lint and test i do before PR 😄

michael-ciniawsky · 2017-05-17T19:59:10Z

lib/index.js

      to: file,
      from: file,
      map: sourceMap
        ? sourceMap === 'inline'
          ? { inline: true, annotation: false }
          : { inline: false, annotation: false }
        : false
-    }, config.options)
+    }, options, config.options)


Will options from webpack.config.js e.g

{ test: /\.css$/, use: [ { loader: 'postcss-loader', options: { parser: 'sugarss' } ] }

be assigned when postcss.config.js is present ?

@michael-ciniawsky using options allow change map, it is bad, for some it is misleading behavior, but to and from now also we can change through config.options. I think about this.

using options allow change map, it is bad, for some it is misleading behavior

? 🙃 You mean that currently one has to set map via { sourceMap: true || 'inline' } (webpack.config.js) exclusively?

but to and from now also we can change through config.options

They shouldn't be changeable, since File I/O is handled by the webpack (Runner) 😛

michael-ciniawsky · 2017-05-17T20:04:47Z

lib/index.js


    return postcssrc(rc.ctx, rc.path, { argv: false })
  }).then((config) => {
    if (!config) config = {}

    if (config.file) this.addDependency(config.file)

+    const sourceMap = options.sourceMap
+    delete options.sourceMap


This caused problems on 'multiple' runs for some weird reason, but maybe because it was defined in the wrong place, not sure here tbh ¯_(ツ)_/¯. It work for the first loader run and was then missing for all further runs, which lead to const sourceMap being always undefined. Besides that delete options.sourceMap => options.sourceMap = null (perf) ?

@michael-ciniawsky weird, should works, I will test in the near future

michael-ciniawsky · 2017-05-17T20:07:35Z

lib/index.js

-      },
-      options: {}
+  Promise.resolve().then(() => {
+    if (!options.config || (options.config && !options.config.path)) {


This changes to config loading behaviour ? See comment below where options get reassigned

@michael-ciniawsky we don't load postcss config file when config not present or present but without path.

postcss-load-config has it's config lookup algo backed in via cosmiconfig, options.config.path should only be used for cases like

|– .config | |– postcss.config.js |– src | |– style.css | |– webpack.config.js

The current trigger for loading postcss.config.js is if no other options are set in webpack.config.js =>postcss.config.js

@michael-ciniawsky current behavior not works as expected we should fix this, see #234 (comment) (point 2).

@michael-ciniawsky two solution:

Load postcss config always and them merge option from loader with config options (i think loader options more important than postcss config option, but ).

Load postcss config only when config: true or config: { path: '/path/to/config'} (best way, no problems with perf and understandable using)

alexander-akait · 2017-05-18T21:35:21Z

@michael-ciniawsky we should finish and merge this PR asap, i spend about 5 hours for migrate to new postcss loader in most of my projects, in half the configuration is not loaded or source maps not working, current loading algorithm is misleading and incorrect.

My solution:

By default config is false and don't load postcss config.
If your have config: true or config: { ctx : {...options}} we loading config (also working good for nested config). Now config: true is disable by schema, but we can fix this.
If your have config: { path: '/path/to/config' } we loading config (absolutly path).

This may seem small breaking changed, but it is bugfix, because now postcss config don't loading in most use case.

alexander-akait · 2017-05-18T21:37:23Z

lib/index.js

-    let options = Object.assign({
+
+    // Disable override `to` option
+    if (config.options.to) delete config.options.to


Disable override important options from postcss config. it is break build.

alexander-akait · 2017-05-18T21:38:13Z

lib/options.js

-    options.parser = params.parser
-    options.syntax = params.syntax
-    options.stringifier = params.stringifier
+  const options = {


We always have params as object, because we have const options = loaderUtils.getOptions(this) || {}

alexander-akait · 2017-05-18T21:41:11Z

lib/index.js

+    // Read options from options.config only when options.config is object
+    if (typeof options.config === 'object' &&
+      options.config !== null &&
+      Object.prototype.toString.call(options.config) === '[object Object]'


Special for config: true

alexander-akait · 2017-05-18T21:57:53Z

lib/options.js

+    parser: params.parser,
+    syntax: params.syntax,
+    stringifier: params.stringifier,
+    map: params.sourceMap


postcss-load-config return map, we should also return map from parseOptions (consistently and don't misleading for future options).

alexander-akait · 2017-05-18T22:10:16Z

lib/index.js


+        return config
+      })
+  }).then((config) => {


config is always object parseOptions return object and postcss-load-config also return object

michael-ciniawsky · 2017-05-19T01:59:23Z

I'm not in favour of changing the config loader behaviour to {config: true }. This neglects the reason for postcss.config.js which is simply to have no in webpack.config.js for most cases at all. I'm also concerned about merging options from (webpack.config.js) && (postcss.config.js). It should be a single source of truth and therefore an either or, I don't see the usecase for this, but if besides an edge case or personal perference one exists we can change it.

Always load config if your pass config.path, now it is not working as expected if i pass config.path and sourceMap: false my config file not loading -> options.config && !sourceMap && length > 1 (i want to load config and set sourceMap use loader options ), also sometimes i want setup some option from options loader.

{ sourceMap: false } is basically unnessecary (default), just omit it, we need to fix the conditional checks for sure agreed upon that, but without breaking the current behaviour

Some perf (we don't create rc object because it is not necessary if i use only options without options.config)

👍

Allow to passing options to postcss throw loader.options, now it is possibly only using postcss config.

What kind of options besides parser || syntax || stringifier && plugins ? The only two suitable options for map are external or inline as an annotation comment, which works via sourceMap: true || 'inline', from, to are handled by webpack aswell

alexander-akait · 2017-05-19T10:32:03Z

@michael-ciniawsky

I'm not in favour of changing the config loader behaviour to {config: true }. This neglects the reason for postcss.config.js which is simply to have no in webpack.config.js for most cases at all. I'm also concerned about merging options from (webpack.config.js) && (postcss.config.js). It should be a single source of truth and therefore an either or, I don't see the usecase for this, but if besides an edge case or personal perference one exists we can change it.

Why now algorithm don't work as expected:

!options.config && !sourceMap && length

I want to use postcss.config.js (options.config is undefined) and disable source map (sourceMap is true), but not setup absolute config path.

options.config && !sourceMap && length > 1

I want to use config (absolutly, options.config is { path: "/path/to/config/"}) and disable source map (sourceMap is false or undefined) (see #234 (comment) point 2).

!options.config && sourceMap && length > 1

Seems works as expected.

options.config && sourceMap && length > 2

I want to use config and have source map, also i have exec. Why my postcss config don't loading.

{ sourceMap: false } is basically unnessecary (default), just omit it, we need to fix the conditional checks for sure agreed upon that, but without breaking the current behaviour

I can't change this in many boilerplate (many of popular them use this option also in config). It is using for all loader options from config.

config.js

module.exports = {
    // ...
    sourceMap: false,
    // ...
};

And in webpack.config.js it is setup to all loaders.

What kind of options besides parser || syntax || stringifier && plugins ? The only two suitable options for map are external or inline as an annotation comment, which works via sourceMap: true || 'inline', from, to are handled by webpack aswell

exec option example. Now also don't work see above.

michael-ciniawsky · 2017-05-20T03:05:43Z

The current loading algo is crap for sure, but still the behaviour shouldn't change :)

❌ { config: true } or similiar isn't an option (neglect's zero config approach)

postcss.config.js

module.exports = ({ file, options, env }) => ({
   // Maybe support options for `map` (PostCSS),
   // but `true` || `inline` handles that fine for the 99% cases,
   // webpack also sets specific requirements here
    sourceMap: options.sourceMap 
});

The reason for not supporting sourceMap within postcss.config.js is during the fact that, it should work with different PostCSS middlewares and therefore only supports PostCSS options && plugins (middleware agnostic). All other middleware related options are set, where the particular middleware normally provides an interface to do so. { sourceMap: true } is a webpack (postcss-loader) specific option and is better defined in webpack.config.js

webpack.config.js

{
   test: '..',
   use: [
     { loader: 'style-loader', options: { sourceMap: env === 'development' ? true : false} },
     { loader: 'css-loader', options: { sourceMap: env === 'development' ? true : false } },
     // { loader: 'postcss-loader', options: { sourceMap: env === 'development' ? true : false } },
     // Usage worst case scenario
     { 
       loader: 'postcss-loader', 
       options: { config: { ctx: { sourceMap: env === 'development' ? true : false } } } 
     },
     { loader: 'sass-loader', options: { sourceMap: env === 'development' ? true : false } }
   ]
}

All we need is to find a better way to filter the loader options and either parse the options, if we have additional ones (parser &&|| plugins), or try to load a config file

alexander-akait · 2017-05-22T12:27:56Z

@michael-ciniawsky ~~Today~~ (sorry, few time 😞 ) I will create a table with all the options and behavior that should, so it will be easier to come to a final decision.

alexander-akait · 2017-05-30T17:18:56Z

@TrySound Maybe you can help us and with this?

alexander-akait · 2017-05-30T20:59:20Z

@michael-ciniawsky now using css inside sass/less/etc throw error, insin/nwb#311 (comment) (https://webpack.slack.com/archives/C3NUYSYF4/p1496168402383007)

alexander-akait · 2017-05-30T21:06:08Z

@michael-ciniawsky also adding any options to options (test: true) break all loading logic.

michael-ciniawsky · 2017-06-14T14:58:45Z

@evilebottnawi Please reduce the PR here only to your proposed sourceMap changes

alexander-akait · 2017-06-14T15:00:00Z

@michael-ciniawsky this PR contain only relevant changed

michael-ciniawsky

How does the sourceMap config in postcss.config.js look like ? A small example please

michael-ciniawsky · 2017-06-14T15:01:55Z

lib/index.js

-    if (options.config.path) {
-      rc.path = path.resolve(options.config.path)
+  Promise.resolve().then(() => {
+    if (options.exec || options.parser || options.syntax || options.stringifier || options.plugins) {


@michael-ciniawsky why? your ugly algo is don't work as expected

michael-ciniawsky · 2017-06-14T15:02:09Z

lib/index.js

    }

-    if (options.config.ctx) {
-      rc.ctx.options = options.config.ctx
+    const rc = {


@michael-ciniawsky we don't need rc if we don't load config

michael-ciniawsky · 2017-06-14T15:05:20Z

package.json

@@ -32,6 +32,7 @@
    "docs": "jsdoc2md lib/index.js > LOADER.md",
    "pretest": "npm run lint && npm run test:build",
    "test": "jest",
+    "test-only": "npm run test:build && jest && npm run clean",


@michael-ciniawsky How can I run only tests?

michael-ciniawsky · 2017-06-14T15:05:27Z

lib/index.js

+    // Disable override `map` option
+    if (config.options.map) delete config.options.map
+
+    let postcssOption = Object.assign({


Why was this renamed? Should be options

@michael-ciniawsky because options related to loader (const options = loaderUtils.getOptions(this) || {}), postcssOptions related to postcss options

It works atm due to lexical scope, as long as there is no need to rename it please leave it as is

michael-ciniawsky · 2017-06-14T15:06:09Z

lib/index.js

    }

    return postcssrc(rc.ctx, rc.path, { argv: false })
-  }).then((config) => {
-    if (!config) config = {}
+      .then((config) => {


wrong ident

@michael-ciniawsky don't understand, please provide example

the code indentation is off :), at least looks it

@michael-ciniawsky

return postcssrc(rc.ctx, rc.path, { argv: false }) .then((config) => { // Get `sourceMap` option from loader options when no `map` option in `postcss.config.js` if (!config.options.map) config.options.map = options.sourceMap return config })

Where?

alexander-akait · 2017-06-14T15:08:40Z

@michael-ciniawsky true || inline

michael-ciniawsky · 2017-06-14T15:12:26Z

But that's not the only two options for map, what happens of e.g { inline: true, annotation: false } is directly passed via postcss.config does it work ?

alexander-akait · 2017-06-14T15:15:46Z

@michael-ciniawsky Do your have specific examples of why this code is not working (example of config or failed tests), all tests are passed, or is this a subjective non-constructive mono solution?

michael-ciniawsky · 2017-06-14T15:22:45Z

Do you have specific examples of why this code is not working (example of config or failed tests), all tests are passed, or is this a subjective non-constructive mono solution?

You mean the config resolving here ?

alexander-akait · 2017-06-14T15:23:27Z

@michael-ciniawsky i mean where algo don't work as expected

alexander-akait · 2017-06-14T15:24:01Z

@michael-ciniawsky #259 Thanks for respect my work and my time.

michael-ciniawsky · 2017-06-14T15:26:59Z

const length = Object.keys(options)
      .filter((option) => {
        // if (option === 'exec') return
        if (option === 'config') return
        if (option === 'sourceMap') return

        return option
      })
      .length

    if (length) {
      return parseOptions.call(this, options)
    }

    const rc = {
      path: path.dirname(file),
      ctx: {
        file: {
          extname: path.extname(file),
          dirname: path.dirname(file),
          basename: path.basename(file)
        },
        options: {}
      }
    }

    if (options.config) {
      if (options.config.path) {
        rc.path = path.resolve(options.config.path)
      }

      if (options.config.ctx) {
        rc.ctx.options = options.config.ctx
      }
    }

    return postcssrc(rc.ctx, rc.path, { argv: false })

You can also take that and I close #259, it was just as another approach to test and discuss for now

https://github.com/postcss/postcss-loader/blob/master/lib/index.js#L99

postcss.config.js

module.exports = (ctx) => ({
  map: { inline: true, annotation: false }
})

Will this work ?

michael-ciniawsky · 2017-06-14T15:31:13Z

Why are you closing this now? 🙃

alexander-akait · 2017-06-14T15:34:48Z

@michael-ciniawsky because I'm tired of non constructive argument and ignoring, for me it will be faster to create fork.

Will this work ?

No, because your say:

The only two suitable options for map are external or inline as an annotation comment, which works via sourceMap: true || 'inline', from, to are handled by webpack aswell

michael-ciniawsky · 2017-06-14T15:52:21Z

What do you mean by non-constructive argument and ignoring ? Ignoring is just plain wrong... Disagreement !== Ignoring

Constrcutive or not is subjective, but

The PR at first changed the config loading enterily and I explained more then once, why I don't think it is a good idea to do it that way
For sourceMap my concern is that there will be confusion about it, when postcss.config.js supports it, but I'm not against it in general (there would still heve been edge case in the PR as is )
The other PR was not meant offensive in any way and I have had no problem by just closing it, I don't care about these things

let options = Object.assign({
      to: file,
      from: file,
      map: sourceMap // <=
        ? sourceMap === 'inline'
          ? { inline: true, annotation: false }
          : { inline: false, annotation: false }
        : false
}, config.options)

This would require additional changes to work with all possible options map PostCSS (postcss.config.js) supports

alexander-akait · 2017-06-14T16:02:11Z

@michael-ciniawsky

Your just close my PR without explaining what is wrong - it is non-constructive.
Your say

The only two suitable options for map are external or inline as an annotation comment, which works via sourceMap: true || 'inline', from, to are handled by webpack aswell

Because i do only true and inline value supporting for sourceMap

Ignoring - refactor: improved loading algorithm of options and configs #234 (comment), refactor: improved loading algorithm of options and configs #234 (comment), refactor: improved loading algorithm of options and configs #234 (comment), refactor: improved loading algorithm of options and configs #234 (comment)
No Respect for my time and effort, thanks

Just don't ignore #259 and merge PR to solve issue

michael-ciniawsky · 2017-06-14T16:17:48Z

kk, can we please in general agree to take it a bit more relaxed and avoid the childish behaviour ?

your just close my PR without explaining what is wrong - it is non-constructive.

When I closed the PR is still changed the loading behaviour, that was the reason

Your say

The only two suitable options for map are external or inline as an annotation comment, which
works via sourceMap: true || 'inline', from, to are handled by webpack aswell
Because I do only true and inline value supporting for sourceMap

Yes, that's the two ways to handle sourcemaps, postcss.config.js supports {Object} e.g { inline: true, annotation: false } which postcss-loader needs handle aswell then, which is ok if needed

Ignoring - #234 (comment), #234 (comment), #234 (comment), #234 (comment)

I answered all of them and the second isn't even addressed at me 🙃

No Respect for my time and effort, thanks

I hearing this for the third+ time now, what about other people's time ? This is just trying to personify matters to passive aggressively offend && push others to do what you want? As I said I don't mind closing my PR's, waste my time in your speaking and in general the least thing I intend to do is to actively disrespect others. Which is disagree I did in anyway, but if you felt that way I apologize

alexander-akait · 2017-06-14T16:21:03Z

@michael-ciniawsky thanks for apologize, let's first fix unexpected behavior with sourceMap #259

alexander-akait · 2017-06-14T16:22:47Z

@michael-ciniawsky and then solve problems: should we respect map from postcss.config.js and how we should do this, or now all ok

michael-ciniawsky · 2017-06-14T16:32:54Z

should we respect map from postcss.config.js

Yes, but we need to add a check / refactor https://github.com/postcss/postcss-loader/blob/master/lib/index.js#L99 properly to support 'full' PostCSS Sourcemap Options {Object}, also the config.options.from/config.options.to sanitizing you added was a good idea

michael-ciniawsky · 2017-06-14T16:33:50Z

Also check if some possible PostCSS Sourcemap Options doesn't play well with webpack (I don't know 💯 yet)

Honestly, I have no idea why this fixes the glitch. But read about it here: webpack-contrib/postcss-loader#234 (comment) 6 And it worked. :|

alexander-akait added semver: patch status: declined type: refactor labels May 16, 2017

alexander-akait requested a review from michael-ciniawsky May 16, 2017 14:25

alexander-akait force-pushed the refactor-options branch from 549488b to 7acce91 Compare May 16, 2017 14:26

michael-ciniawsky reviewed May 17, 2017

View reviewed changes

michael-ciniawsky added this to the 2.0.6 milestone May 17, 2017

michael-ciniawsky added priority: medium ci: ok labels May 17, 2017

michael-ciniawsky changed the title ~~refactor: improved loading algorithm of options and configs.~~ refactor: improved loading algorithm of options and configs May 17, 2017

alexander-akait force-pushed the refactor-options branch 2 times, most recently from e794dab to bce5503 Compare May 18, 2017 21:29

alexander-akait force-pushed the refactor-options branch 2 times, most recently from fab9590 to 62e2a2b Compare May 18, 2017 21:42

alexander-akait commented May 18, 2017

View reviewed changes

alexander-akait force-pushed the refactor-options branch from 62e2a2b to 96132e3 Compare May 18, 2017 21:53

alexander-akait commented May 18, 2017

View reviewed changes

alexander-akait force-pushed the refactor-options branch 3 times, most recently from c8848bd to 75965c0 Compare May 18, 2017 22:09

alexander-akait commented May 18, 2017

View reviewed changes

michael-ciniawsky requested changes Jun 14, 2017

View reviewed changes

refactor: improved loading algorithm of options and configs.

22ed94f

alexander-akait force-pushed the refactor-options branch 2 times, most recently from 531041a to 22ed94f Compare June 14, 2017 15:18

alexander-akait closed this Jun 14, 2017

alexander-akait deleted the refactor-options branch June 14, 2017 15:24

alexander-akait mentioned this pull request Jun 14, 2017

fix: sanitizing from and to options (postcss.config.js) #260

Merged

michael-ciniawsky modified the milestone: 2.0.7 Jun 14, 2017

refactor: improved loading algorithm of options and configs #234

refactor: improved loading algorithm of options and configs #234

Conversation

alexander-akait commented May 16, 2017

alexander-akait commented May 17, 2017

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

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

alexander-akait commented May 18, 2017 • edited Loading

alexander-akait May 18, 2017 • edited Loading

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

michael-ciniawsky commented May 19, 2017

alexander-akait commented May 19, 2017 • edited Loading

michael-ciniawsky commented May 20, 2017 • edited Loading

alexander-akait commented May 22, 2017 • edited Loading

alexander-akait commented May 30, 2017

alexander-akait commented May 30, 2017

alexander-akait commented May 30, 2017

michael-ciniawsky commented Jun 14, 2017

alexander-akait commented Jun 14, 2017

michael-ciniawsky left a 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

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

michael-ciniawsky Jun 14, 2017 • edited Loading

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

Choose a reason for hiding this comment

alexander-akait commented Jun 14, 2017

michael-ciniawsky commented Jun 14, 2017

alexander-akait commented Jun 14, 2017 • edited Loading

michael-ciniawsky commented Jun 14, 2017

alexander-akait commented Jun 14, 2017 • edited Loading

alexander-akait commented Jun 14, 2017

michael-ciniawsky commented Jun 14, 2017 • edited Loading

michael-ciniawsky commented Jun 14, 2017

alexander-akait commented Jun 14, 2017 • edited Loading

michael-ciniawsky commented Jun 14, 2017 • edited Loading

alexander-akait commented Jun 14, 2017 • edited Loading

michael-ciniawsky commented Jun 14, 2017 • edited Loading

alexander-akait commented Jun 14, 2017

alexander-akait commented Jun 14, 2017

michael-ciniawsky commented Jun 14, 2017 • edited Loading

michael-ciniawsky commented Jun 14, 2017

alexander-akait commented May 18, 2017 •

edited

Loading

alexander-akait May 18, 2017 •

edited

Loading

alexander-akait commented May 19, 2017 •

edited

Loading

michael-ciniawsky commented May 20, 2017 •

edited

Loading

alexander-akait commented May 22, 2017 •

edited

Loading

michael-ciniawsky Jun 14, 2017 •

edited

Loading

alexander-akait commented Jun 14, 2017 •

edited

Loading

alexander-akait commented Jun 14, 2017 •

edited

Loading

michael-ciniawsky commented Jun 14, 2017 •

edited

Loading

alexander-akait commented Jun 14, 2017 •

edited

Loading

michael-ciniawsky commented Jun 14, 2017 •

edited

Loading

alexander-akait commented Jun 14, 2017 •

edited

Loading

michael-ciniawsky commented Jun 14, 2017 •

edited

Loading

michael-ciniawsky commented Jun 14, 2017 •

edited

Loading