search

bholloway / resolve-url-loader

Webpack loader that resolves relative paths in url() statements based on the original source file
563 stars 70 forks source link

Explicit sourceMap=false is sometimes ignored #60

Closed ertrzyiks closed 7 years ago

ertrzyiks commented 7 years ago

Because of the way useMap flag is evaluated: index.js#L100

var useMap = loader.sourceMap || options.sourceMap

explicitly setting sourceMap option to false means nothing if loader.sourceMap is true. If any plugin sets that flag to true then we can not disable generating source maps. Lack of control over this flag may lead for example to a warning in postcss-loader (see postcss/postcss-loader#253).

DorianGrey commented 7 years ago

I'd suggest something like

var useMap = options.sourceMap == null ? loader.sourceMap : options.sourceMap

should be preferable - i.e. in case the options.sourceMap option was set explicitly, it should take precedence over loader.sourceMap.

bholloway commented 7 years ago

Ok, sorry, a fix to this is overdue. I have some time now.

Meanwhile can I get some thoughts on semver for this change?...

I know if is "more correct" to fix this, but if it is released as a patch version I am concerned it may break anyone who (unwittingly) is relying on this bug to give source-maps.

I am thinking a minor version change. I don't think it is worth a major version change.

bholloway commented 7 years ago

I'm proposing PR #61. Please let me know your thoughts on that PR.

DorianGrey commented 7 years ago

Personally, I'd put this in patch version, since it's a bug. This bug only occurs in case loader.sourceMap is truthy, while options.sourceMap is falsy. Regularly, the loader will be chained with css-loader or postcss-loader, which will either complain about an attached sourcemap and warn that it'll get dropped (postcss-loader), or will just drop it without further notice (css-loader). In both terms, you won't get a sourcemap anyway. Also, all loaders I know of mention that source maps have to enabled explicitly, so it would be curious to find someone relying on this side-effect. Considering these circumstances, fixing the bug will fix most warnings produced by build chains, but should not have a reasonable technical impact.

bholloway commented 7 years ago

Published resolve-url-loader@2.0.3.

Go forth without source-maps @DorianGrey @ertrzyiks @larssn !