feat: recognise config.options.map (options.sourceMap)

[Justineo]

Currently loader options can be overridden by config options loaded by postcss-load-config.

So we should use the merged options.map here instead of sourceMap.

[alexander-akait]

@michael-ciniawsky What do your think about this? Should we prioritize loader options over config?

[Justineo]

@evilebottnawi

This is not prioritizing loader options over config. Loader options are already extended by config here:

https://github.com/postcss/postcss-loader/blob/271ab9a86799a97e3b230815e8365728b4072615/lib/index.js#L101-L109

This PR is just making sure that we can enable source map from config.

Although I think loader options should have higher priority, considering changing this might include a big breaking change, I'm not gonna suggest you to change current logic.

[michael-ciniawsky]

The reason I avoided this is that in a normal setup you need to enable sourceMap in various places (style-loader, css-loader, sass-loader etc) to work correctly and all possible PostCSS Sourcemap Options are not 1:1 compatible with how webpack handles sourcemaps internally. config.options.map would need to have a safeguard at least to ensure it is either { inline: false, annotation: false } || { inline: true , annotation: false }. The current approach in this PR can lead to subtile bugs. I'm not really in favour of this change tbh.

[michael-ciniawsky]

This also needs a (separate) test

[michael-ciniawsky]

Needs tests and a safeguard to ensure config.options.map is consistent with the two sourcemap options currently possible for webpack setups ( true|'inline')

Since neither true nor inline exists as bare PostCSS Options directly, which postcss.config.js needs to support without any abstraction to be consitent across all postcss runners (CLI, Gulp, webpack), there needs to be a safeguard ensuring both ways of configuration behave exactly the same.

[Justineo]

Needs tests and a safeguard to ensure config.options.map is consistent with the two sourcemap options currently possible for webpack setups ( true|'inline')

I'm not sure if I understand this correctly:

Because postcss-loader only support true and 'inline' as the value of sourceMap option, which is converted into { inline: false, annotation: false } and { inline: true, annotation: false } as the value of map option for Postcss, we have to make sure people won't configure something like { annotation: true } in config files if we are gonna accept map options directly from config files.

[Justineo]

What I'm confusing about here is postcss-loader prioritize some options from config files but ignore some other options from them at all. I have to configure in both places and be aware of the details under the hood if I want to put things into config files.

[michael-ciniawsky]

What I'm confusing about here is postcss-loader prioritize some options from config files but ignore some other options from them at all.

options.from && options.to are handled by webpack as it is resposible for File I/O. For using the bare PostCSS Node API + postcss-load-config options.from && options.to can be useful, albeit the usecase is even limited there. For postcss-loader or gulp-postcss these two options don't make sense at all so they are ignored if set via config

I have to configure in both places and be aware of the details under the hood if I want to put things into config files.

Yeah agreed, it is in one way or the other not optimal, but the standard way for sourceMap in webpack.config.js is { sourceMap: {Boolean} } and webpack sets specific requirements of how a sourcemap file needs to look like to work correctly, so not all supported options.map values are valid for postcss-loader, but are for postcss.config.js

webpack.config.js (No options.map possible [Current])

[
   { loader: 'style-loader', options: { sourceMap: true } },
   { loader: 'css-loadder', options: { sourceMap: true } },
   { loader: 'postcss-loader', options: { sourceMap: true } },
   { loader: 'sass-loader', options: { sourceMap: true } }
]

webpack.config.js (with options.map possible [Proposed])

[
   { loader: 'style-loader', options: { sourceMap: true } },
   { loader: 'css-loadder', options: { sourceMap: true } },
   'postcss-loader', // <= "Inconsistent" for options.sourceMap (as the point of view) 
   { loader: 'sass-loader', options: { sourceMap: true } }
]

Since { sourceMap: .. } needs to be set on all loaders within a rule, the optioniated decision was to only allow sourceMap to be set via webpack.config.js for consistency reasons (nits), since both approaches are not 💯 sufficient anyways.

[Justineo]

Thanks for the explanation. So rather than changing current behavior, I think we'd better clarify this in the docs to prevent confusion like "I've configured source maps in my config file, why do postcss-loader still giving warnings?".

[michael-ciniawsky]

@Justineo I gave this some though and this two paths are possible

L99+

Warning

if (config.options.map && sourceMap) delete config.options.map

if (config.options.map && !sourceMap) {
  this.emitWarning(`\n\n ⚠️  PostCSS Loader\n\nSetting source map via 'postcss.config.js' isn't supported due to inconsistent behaviour between source map options, please use options.sourceMap in 'webpack.config.js' instead.\nThe loader will not generate a source map with the current configuration.\nSee https://github.com/postcss/postcss-loader#sourcemap for more information.\n\n`)

  delete config.options.map
}

Fallback (no inline)

if (config.options.map && sourceMap) delete config.options.map

 if (config.options.map && !sourceMap)  {
  delete config.options.map
  sourceMap = true // !  Needs a change from const to let for sourceMap (L50)
}

Or a mixture of both, totally open for suggestions here cc @evilebottnawi 😛

[Justineo]

@michael-ciniawsky
I think updating warning should be enough for my case. The fallback solution may add extra complication here IMO. I'm closing this PR and thanks for your explanation. 👍