fix(image): url-encode SVG source

[olance]

Rollup Plugin Name: image

This PR contains:

• bugfix
• feature
• refactor
• documentation
• other

Are tests included?

• yes (bugfixes and features will not be merged without tests)
• no

Given the current test suite of the plugin, I'm not sure there's anything to change in the tests: all they seem to do is check that the import statements work correctly, without checking the actual resulting code (?)

Breaking Changes?

• yes (breaking changes will not be merged unless absolutely necessary)
• no

List any relevant issue numbers:

Description

As per the spec, any source (to reuse the code's variable name) that is not base64-encoded must be URL-encoded for the data: URI to be valid.

With the current version of the plugin, I'm getting:

<img src="data:image/svg+xml;utf-8,<svg xmlns=&quot;http://www.w3.org/2000/svg&quot; width=&quot;88&quot; height=&quot;36&quot; viewBox=&quot;0 0 88 36&quot;><path fill=&quot;#3651D2&quot; d=&quot;M1 31h87v5H1z&quot; /><path fill=&quot;#262626&quot; d=&quot;M13.008 11.85v-1.71h4.922v15.247h-4.922v-1.846C11.674 25.592 9.76 25.9 8.495 25.9c-2.188 0-4.034-.547-5.709-2.325C1.18 21.866.564 19.883.564 17.866c0-2.563.957-4.751 2.359-6.153 1.333-1.333 3.18-2.085 5.299-2.085 1.299 0 3.384.307 4.786 2.222zm-3.556 2.017c-1.06 0-2.05.444-2.7 1.094-.513.512-1.129 1.401-1.129 2.769 0 1.367.582 2.256 1.026 2.735.684.717 1.743 1.196 2.906 1.196.991 0 1.914-.41 2.564-1.06.65-.615 1.196-1.572 1.196-2.871 0-1.094-.41-2.12-1.128-2.804-.752-.717-1.846-1.06-2.735-1.06zM25.612.466h4.922v24.921h-4.922V.466zm12.604 24.921V10.141h4.922v1.777c1.265-1.914 3.111-2.154 4.274-2.154.65 0 1.846.069 2.871.684.786.445 1.436 1.197 1.846 2.12a5.82 5.82 0 0 1 1.949-1.949c1.06-.65 2.05-.855 3.247-.855 1.846 0 3.35.547 4.274 1.436 1.47 1.402 1.572 3.453 1.572 4.547v9.64h-4.923v-7.726c0-.786 0-2.153-.65-2.974-.41-.513-1.059-.82-1.777-.82-.82 0-1.504.376-1.914.923-.65.82-.752 2.017-.752 3.008v7.59h-4.923v-7.932c0-1.025-.034-2.222-.786-2.974-.513-.513-1.128-.615-1.641-.615-.65 0-1.23.17-1.71.615-.922.889-.957 2.598-.957 3.11v7.795h-4.922zM82.44 11.85v-1.71h4.923v15.247h-4.923v-1.846c-1.333 2.051-3.247 2.359-4.512 2.359-2.188 0-4.034-.547-5.71-2.325-1.606-1.709-2.221-3.692-2.221-5.709 0-2.563.957-4.751 2.359-6.153 1.333-1.333 3.179-2.085 5.298-2.085 1.3 0 3.385.307 4.786 2.222zm-3.555 2.017c-1.06 0-2.051.444-2.7 1.094-.513.512-1.129 1.401-1.129 2.769 0 1.367.581 2.256 1.026 2.735.683.717 1.743 1.196 2.906 1.196.99 0 1.914-.41 2.563-1.06.65-.615 1.197-1.572 1.197-2.871 0-1.094-.41-2.12-1.128-2.804-.752-.717-1.846-1.06-2.735-1.06z&quot;/></svg>">

Which renders as a broken image in latest Chrome.

With the proposed change, I'm getting:

<img src="data:image/svg+xml;utf-8,%3Csvg%20xmlns%3D%22http%3A%2F%2Fwww.w3.org%2F2000%2Fsvg%22%20width%3D%2288%22%20height%3D%2236%22%20viewBox%3D%220%200%2088%2036%22%3E%3Cpath%20fill%3D%22%233651D2%22%20d%3D%22M1%2031h87v5H1z%22%20%2F%3E%3Cpath%20fill%3D%22%23262626%22%20d%3D%22M13.008%2011.85v-1.71h4.922v15.247h-4.922v-1.846C11.674%2025.592%209.76%2025.9%208.495%2025.9c-2.188%200-4.034-.547-5.709-2.325C1.18%2021.866.564%2019.883.564%2017.866c0-2.563.957-4.751%202.359-6.153%201.333-1.333%203.18-2.085%205.299-2.085%201.299%200%203.384.307%204.786%202.222zm-3.556%202.017c-1.06%200-2.05.444-2.7%201.094-.513.512-1.129%201.401-1.129%202.769%200%201.367.582%202.256%201.026%202.735.684.717%201.743%201.196%202.906%201.196.991%200%201.914-.41%202.564-1.06.65-.615%201.196-1.572%201.196-2.871%200-1.094-.41-2.12-1.128-2.804-.752-.717-1.846-1.06-2.735-1.06zM25.612.466h4.922v24.921h-4.922V.466zm12.604%2024.921V10.141h4.922v1.777c1.265-1.914%203.111-2.154%204.274-2.154.65%200%201.846.069%202.871.684.786.445%201.436%201.197%201.846%202.12a5.82%205.82%200%200%201%201.949-1.949c1.06-.65%202.05-.855%203.247-.855%201.846%200%203.35.547%204.274%201.436%201.47%201.402%201.572%203.453%201.572%204.547v9.64h-4.923v-7.726c0-.786%200-2.153-.65-2.974-.41-.513-1.059-.82-1.777-.82-.82%200-1.504.376-1.914.923-.65.82-.752%202.017-.752%203.008v7.59h-4.923v-7.932c0-1.025-.034-2.222-.786-2.974-.513-.513-1.128-.615-1.641-.615-.65%200-1.23.17-1.71.615-.922.889-.957%202.598-.957%203.11v7.795h-4.922zM82.44%2011.85v-1.71h4.923v15.247h-4.923v-1.846c-1.333%202.051-3.247%202.359-4.512%202.359-2.188%200-4.034-.547-5.71-2.325-1.606-1.709-2.221-3.692-2.221-5.709%200-2.563.957-4.751%202.359-6.153%201.333-1.333%203.179-2.085%205.298-2.085%201.3%200%203.385.307%204.786%202.222zm-3.555%202.017c-1.06%200-2.051.444-2.7%201.094-.513.512-1.129%201.401-1.129%202.769%200%201.367.581%202.256%201.026%202.735.683.717%201.743%201.196%202.906%201.196.99%200%201.914-.41%202.563-1.06.65-.615%201.197-1.572%201.197-2.871%200-1.094-.41-2.12-1.128-2.804-.752-.717-1.846-1.06-2.735-1.06z%22%2F%3E%3C%2Fsvg%3E">

Which renders correctly.

[shellscape]

Thanks for the PR on this.

So one of the great reasons we have CI in place to run tests for pull requests is that they often reveal that while a particular fix is valid for a certain scenario, it can break for others. CI failing here is showing you that. Your fix works for consuming the result for the DOM, but not when not wanting to use the DOM.

That's also a good example for why we require tests for new fixes. Tests not only demonstrate why a fix was created and committed, but also provide future-proofing against regressions. If we were to merge this, we introduce a regression.

I think this is still worth fixing, but it needs to be done in a different way - encode while setting img.src but not otherwise. A new test for this should use { dom: true } with an unencoded svg and then snapshot the result to assert proper encoding.

[olance]

Hey @shellscape, thanks for looking at this so quickly :)

I had not gathered that the "snapshot" part was actually going to perform checks of expected output.
I am 100% aligned with the necessity of tests and I had not included one because I had wrongly assumed this particular plugin was not really tested thoroughly 🤦‍♂

So I'll push a test then 🙂

However I'm not sure I agree with your comment on the fix being specific to { dom: true }.
Am I correct when I say that when not using the dom: true option, the result of the import is still a data: URL meant to be used as the source of an image element?
Even if not directly used with a DOM element, the RFC 2397 for data: URLs specifies that the urlchar part of the URL is indeed a URI:

3. Syntax

       dataurl    := "data:" [ mediatype ] [ ";base64" ] "," data
       mediatype  := [ type "/" subtype ] *( ";" parameter )
       data       := *urlchar
       parameter  := attribute "=" value

   where "urlchar" is imported from [RFC2396], 

👉 RFC2396 - Uniform Resource Identifiers (URI): Generic Syntax

As such, I believe the snapshot that current tests are using to check for the correct import of images without { dom: true } are not valid, since characters like <, ", > and so on found in the XML content of the SVG files are not URI-compatible, and thus must be escaped.

I'm happy to include all fixes required in that PR, however I am not sure how I should go about generating the correct snapshots to test against? Sorry for the noobiness here...

[shellscape]

It appears that plugin-url is taking that approach as well.

plugins/packages/url/src/index.js

Lines 66 to 69 in b3d0905

	const isSVG = mimetype === 'image/svg+xml';
	data = isSVG ? encodeSVG(buffer) : buffer.toString('base64');
	const encoding = isSVG ? '' : ';base64';
	data = `data:${mimetype}${encoding},${data}`;

So I'd say that's valid given the information you provided and the prior work in the other plugin.

Please add/update tests and we'll look at merging this.

[wuweiweiwu]

LGTM!

[wuweiweiwu]

very tiny nit: can we pull these out into consts or enums?

[olance]

By "these", do you mean the mime types?

[olance]

Please add/update tests and we'll look at merging this.

Thanks @shellscape I will, would you mind just giving me pointers on this:

I'm happy to include all fixes required in that PR, however I am not sure how I should go about generating the correct snapshots to test against? Sorry for the noobiness here...

Thanks!

[rsmogura]

Hi,

Sorry for jumping in, and if you want please ignore my comment.

I see that with this change, the SVG content will be encoded using URL encoding. I think this will increase size of inlined resource.

I wonder if it's possible to store the SVG in its original form, and then transform it to base64 data URI using btoa function on the client side.

This way we would keep data small and overcome issues with ending of data URI.

[shellscape]

@rsmogura yes, it will some. This is a good point and worth talking about.

The example in the OP results in a:

• 248 character count increase: encoding versus no encoding
• 245 character count increase: base64 versus encoding
• 493 character count increase: base64 versus no encoding

Using btoa is a good option for when dom: true is used, but that leaves the question of how to handle non-dom applications. There are a few scenarios to consider:

1. dom: true
2. dom: false but used in a browser environment
3. dom: false but used in a Node environment

We have 1 covered by this PR.

We have to consider what to do for number 2. We could still use btoa in this case, but we have no way to know if the bundle is being used in a browser.

We have to consider what to do for number 3. For Node, we can simply convert using Buffer similar to btoa. We don't know when the user is targeting Node and when they're not.

[rsmogura]

For fronted (browser) size would be very important.

In ideal world it would be good if plugin could register own 'global' function "something" like

encodeDataUriSafe(mediaType, data) {
  if (Buffer) return `data:${mediaType};base64,${new Buffer(data).toString(...)};
  if (btoa) return `data:${mediaType};base64,${btoa(data)}`;
  return `data:${mediaType},${escape(data)}`
}

Than instead of guessing what environment we are in, we could use it for every generated code for every image?

[shellscape]

Eh that's not a good pattern for a plugin. Might be on the framework level, but not for the plugins here. It'd be better to have the user instruct the plugin, or better yet Rollup, on how to behave. @lukastaegert any thoughts?

[rsmogura]

@shellscape,

I wonder if we could use encodeURI it's built-in in Node (even not stated in documentation)?

[dxlbnl]

EncodeURI doesn't encode < and > characters. which it should. encode and decode do, and ot my knowledge, have the right behaviour

[shellscape]

I think I've arrived at a good solution for all scenarios. Will update the PR branch soon.

[shellscape]

I pushed my changes to get this working. The big change here is that we're using URLSearchParams to encode anything that's an svg and not dom: true. For dom: true svg, we're going to use btoa and hardcode the format to base64. URLSearchParams is available as a global in both Node and the browser.

For those concerned about bundle size, make sure you inspect the snapshot-markdown files to see how the svg is represented in the bundle.

The only catch is that URLSearchParams is not available in Internet Explorer https://caniuse.com/#feat=urlsearchparams, and users wishing to support IE that are not using dom: true will have to include the polyfill (https://github.com/jerrybendy/url-search-params-polyfill) that adds another 10KB to an unminified, ungzipped bundle. The minified, gzipped size of the polyfill is 1341 bytes. To me, that's acceptable. Willing to hear thoughts on that.

@rsmogura @neoel @wuweiweiwu @olance

[Andarist]

I just take a quick look, so I might definitely miss something - but wouldn't running SVG through https://github.com/tigt/mini-svg-data-uri just solve this? No runtime code (for the conversion) would have to be included in the output, just appropriately encoded source, which (using this technique) would be barely larger than the input SVG?

[shellscape]

This tool converts SVGs into the most compact, compressible data: URI that SVG-supporting browsers tolerate.

This plugin doesn't aim to modify the image that's imported. That module would modify the source, not something we want to get into the business of for this one.

[Andarist]

It has some caveats - they are listed on the README. I wasnt implying that we have to use this particular tool (even though I've written it like I was 😅 ), but rather reuse its technique to avoid relying on URLSearchParams. I'm not entirely sure if that is doable, but at first glance - it should be (that library is not optimizing much, it's really mostly about encoding)

[rsmogura]

Hi @shellscape,

Thank you for this commit.

I download PR and checked on Safari and Chrome using const encoding. In my case images didn't load. However when I changed escaping function to encodeURIComponent it fixed issue (sorry for mentioning previously encodeURI).

I experimented a little bit with "virtual modules" and imports, so "escape" function will be added only once - rsmogura@63e310a - I think with this approach I reduced size of JS as I loaded multiple SVGs in it.

[shellscape]

I tested the output of all 5 tests on Firefox, Safari, Edge, and Chrome. You must have done something wrong on your end.

[rsmogura]

I’ll check it tomorrow again. In my case I pass a data URI as an image source in React tree.

[shellscape]

@Andarist I see what you're saying now that I'm off my mobile. The idea we were going for was to pass the svg to the bundle as-is, and then encode it within the bundle. Attempting to achieve smallest possible footprint. That may not work out for us in the long run, but it's worth a shot.

[rsmogura]

@shellscape

So I checked again, the SVG is converted to following dataURI which can't be opened by Safari, nor Chrome.

data:image/svg+xml;utf-8,%3C%3Fxml+version=%221.0%22+encoding%3D%22UTF-8%22%3F%3E%3Csvg+width%3D%2246px%22+height%3D%2245px%22+viewBox%3D%220+0+46+45%22+version%3D%221.1%22+xmlns%3D%22http%3A%2F%2Fwww.w3.org%2F2000%2Fsvg%22+xmlns%3Axlink%3D%22http%3A%2F%2Fwww.w3.org%2F1999%2Fxlink%22%3E++++%3C%21--+Generator%3A+Sketch+62+%2891390%29+-+https%3A%2F%2Fsketch.com+--%3E++++%3Ctitle%3ESlice+1%3C%2Ftitle%3E++++%3Cdesc%3ECreated+with+Sketch.%3C%2Fdesc%3E++++%3Cg+id%3D%22Page-1%22+stroke%3D%22none%22+stroke-width%3D%221%22+fill%3D%22none%22+fill-rule%3D%22evenodd%22%3E++++++++%3Cpolygon+id%3D%22Path%22+stroke%3D%22%23979797%22+points%3D%221.62890625+22.6171875+45.390625+43.3203125+45.390625+1.21484375%22%3E%3C%2Fpolygon%3E++++%3C%2Fg%3E%3C%2Fsvg%3E

With encodeURIComponent it has form

data:image/svg+xml;utf-8,%3C%3Fxml%20version%3D%221.0%22%20encoding%3D%22UTF-8%22%3F%3E%3Csvg%20width%3D%2246px%22%20height%3D%2245px%22%20viewBox%3D%220%200%2046%2045%22%20version%3D%221.1%22%20xmlns%3D%22http%3A%2F%2Fwww.w3.org%2F2000%2Fsvg%22%20xmlns%3Axlink%3D%22http%3A%2F%2Fwww.w3.org%2F1999%2Fxlink%22%3E%20%20%20%20%3C!--%20Generator%3A%20Sketch%2062%20(91390)%20-%20https%3A%2F%2Fsketch.com%20--%3E%20%20%20%20%3Ctitle%3ESlice%201%3C%2Ftitle%3E%20%20%20%20%3Cdesc%3ECreated%20with%20Sketch.%3C%2Fdesc%3E%20%20%20%20%3Cg%20id%3D%22Page-1%22%20stroke%3D%22none%22%20stroke-width%3D%221%22%20fill%3D%22none%22%20fill-rule%3D%22evenodd%22%3E%20%20%20%20%20%20%20%20%3Cpolygon%20id%3D%22Path%22%20stroke%3D%22%23979797%22%20points%3D%221.62890625%2022.6171875%2045.390625%2043.3203125%2045.390625%201.21484375%22%3E%3C%2Fpolygon%3E%20%20%20%20%3C%2Fg%3E%3C%2Fsvg%3E

[shellscape]

OK, that's an SVG that was mangled then. We're going to go with @Andarist recommendation. While it will result in a slightly larger SVG in the bundle, it's the safest. Folks will have to optimize before bundling.

[shellscape]

@rsmogura please try the new code just pushed

[rsmogura]

@shellscape LGTM! Thanks!

[olance]

@shellscape & all, thanks for keeping this alive and, in the end, finishing the job!
I'm sorry things got intense at work and I couldn't follow-up on the discussions/suggestions... but I'm glad it's been seen through and merged, thanks! 🙂

[shellscape]

No worries, life happens!