svg-url-loader works incorrectly when load images not from CSS files.

[constgen]

The issue:

https://mega.nz/#!KZFlCD6B!ziIF6WMlEArcD8RPJChIsteJLIQfDgzKmlTxXyUlSxI
This is a simple project that demonstrates the problem with svg-url-loader.

It works as expected when SVG images are referenced from CSS code. But when load image with import and inject it in HTML with interpolation or by reference in JSX it works incorrectly. It is caused by wrapping quotes around resulting string "...". Need to strip this quotes every time when you load SVG in JS

import logoSvg from './images/logo.svg'
const imageSvg = JSON.parse(logoSvg)

It is expected that svg-url-loader will work the same way as image-loader

The solution:

Webpack loader config has an "issuer" property. There is a good example of usage in the Less loader https://webpack.js.org/loaders/less-loader/#non-less-imports . We can pass different config to svg-url-loader dependeing on a dependency parent. svg-url-loader supports "noquotes" option that disables that quotes.

But webpack-chain doesn't support "issuer" property, that's why I propose to add it. @eliperelman

Also there is a chance that we will have to contribute to svg-url-loader and add an additional option.

[eliperelman]

Note: webpack-chain supports issuer, it just doesn't have a shorthand method for it:

neutrino.config.rule(name).set('issuer', ...)

Also, feel free to PR webpack-chain and add issuer to Rule extensions.

[constgen]

Ok, I will investigate it.

[eirikurn]

This is affecting us badly. @constgen did you get this to work with issuer?

Does it make sense to revert to file-loader, which "just works", until this is solved?

[eliperelman]

@timkelty you did the original PR for switching to svg-url-loader. What is your opinion here?

[timkelty]

Hmm - yeah I was under the impression svg-url-loader was a drop-in replacement, but apparently it isn't.

Sounds like maybe it is a good idea to revert back to file-loader until we figure out the proper fix.

[constgen]

Started working on it. Currently have an issue with separating { noquotes: true } and { noquotes: false} configs. Seams like a bug in Webpack but I am not sure. This is what I tried:

  config.module
    .rule('svg')
    .set('issuer', /\.(js|jsx|ts|tsx|html|vue)$/)
    .test(/\.svg(\?v=\d+\.\d+\.\d+)?$/)
    .use('url')
      .loader(svgUrlLoader)
      .options(merge({ limit }, options.svg || {}))
      .tap(opts => merge(opts, { noquotes: true }));

  config.module
    .rule('style-svg')
    .set('issuer', /\.(css|less|sass|scss)$/)
    .test(/\.svg(\?v=\d+\.\d+\.\d+)?$/)
    .use('url')
      .loader(svgUrlLoader)
      .options(merge({ limit }, options.svg || {}))
      .tap(opts => merge(opts, { noquotes: false }));

Only the latest rule is applied correctly. Changing order of rules affects the result. This is not expected to be. It may take several days to fix.

Have one more idea with json-loader

[edmorley]

In addition to the use-case of importing SVGs directly in a project's JS, this issue also affects HTML assets loaded by html-loader that refer to SVGs (for example when passing a custom template to html-webpack-plugin).

It seems really broken for consumers of svg-url-loader to have to know whether to pass in noquotes or not - particularly for the html-loader use-case. As such for that I've filed:
bhovhannes/svg-url-loader#126

Given that svg-url-loader is not the drop-in replacement it first appeared, and that IMO consumers shouldn't have to be working around the quoting bugs - my preference would be for Neutrino to stop using it for now.

[constgen]

The new test suite is available on the same link https://mega.nz/#!KZFlCD6B!ziIF6WMlEArcD8RPJChIsteJLIQfDgzKmlTxXyUlSxI . You need to link the Neutrino locally and switch to bugfix/svg-loader branch to see the results.

There are bad and good news.

• Good: the new loader config uses different settings in styles and rest of module types to correctly provide image URL. Background, HTML, imports in JS and JSX - all these works as expected.
• Bad: if you try to import or use the same image in styles and JS the image will be displayed correctly only in one case: CSS or JS. Its random and depends on which version was parsed first and cached (or last, I am not sure)

So in the test suite you can see that I used the copy of the image in the CSS to avoid this problem: background-image: url(./images/logo-copy.svg) isntead of background-image: url(./images/copy.svg). Adding random query param will fix the issue too background-image: url(./images/copy.svg?random). It seems to be a Webpack module caching issue. It doesn't consider that the same resources was required from different issuers. Or it is by design. May be somebody can explain.

[eliperelman]

@constgen would this problem go away if we used file/url-loader instead?

[constgen]

Yes, it would. But we need to provide an alternative for those who uses SVG markup for inlined isons.

[eliperelman]

Not sure what you mean, can you elaborate?

[edmorley]

So I've spent some more time digging into this...

I've come to the conclusion that svg-url-loader shouldn't ever be adding quotes to the data URI string it returns, and that the only reason it does was to work around a bug in css-loader that means imported data URIs aren't correctly wrapped in quotes like inline data URIs are. (It's not clear whether the svg-url-loader author realised this was a css-loader bug or not.)

I've created a reduced STR at webpack-contrib/css-loader/issues/615 and once/if that's fixed, the whole quoting and noquotes behaviour can be removed entirely from svg-url-loader, avoiding all of these workarounds.

Given this, I don't think we should add any workarounds in Neutrino for now, and should instead switch back to url-loader (ie revert #171) since it's definitely not the drop-in replacement for url-loader we thought it was at the time.

Also something I forgot to mention in my last comment -- even when using noquotes the resultant HTML then fails validation with this plugin, since the src value contains spaces. ie: another way in which this loader isn't a drop-in replacement.

[eliperelman]

@edmorley wow, great analysis, thank you so much!

Luckily, I decided I revert back to using url-loader for SVGs right before I released Neutrino v7, so we should be safe there right now.

Closing, and we can add the svg-url-loader back when other packages get updated.

[timkelty]

Thanks to all, and sorry for causing this headache initially!

[eliperelman]

@timkelty no worries at all, it's a great learning experience for all of us!

[constgen]

Can we merge my changes in v6? At least they will resolve some problems.