Not compatible in srcset attribute

[polarathene]

In the gatsby-image project, we've discovered in a PR being reviewed that SVG data URIs from this library are not compatible with the srcset attribute.

Only base64 or url-encoded work. The only notable difference from the example given, was literal spaces vs %20. In the srcset attribute, you provide a comma delimited list of URL/URIs and an optional space followed by a descriptor(eg 1x, 100w), if none provided they'll use a default.

With the output from mini-svg-data-uri a console error is raised, and the srcset is ignored.

---

It might be worth mentioning on your README that mini-svg-data-uri will not work for this use-case? Or at least requires another pass to convert the spaces to %20, or this library provides some config/option to handle that?(it looks like you intentionally convert to literal spaces?)

[tigt]

Oh, interesting. Thank you for bringing this up.

I can see the spaces being important since part of the srcset algorithm involves whitespace separators.

My first inclination is to export a toSrcSet option on the main object, but do you have an opinion on that?

[polarathene]

I like it, the intent is clear for handling the srcset attribute instead of regular src.

This is the current workaround(encodeSpaces) for where it's being used with Gatsby:

 const svgToMiniDataURI = require(`mini-svg-data-uri`)
 const potrace = require(`potrace`)
 const trace = promisify(potrace.trace)

 const defaultArgs = {
   color: `lightgray`,
   optTolerance: 0.4,
   turdSize: 100,
   turnPolicy: potrace.Potrace.TURNPOLICY_MAJORITY,
 }

 const optionsSVG = _.defaults(args, defaultArgs)

 // `srcset` attribute rejects URIs with literal spaces
 const encodeSpaces = (str) => str.replace(/ /gi, `%20`)

 return trace(tmpFilePath, optionsSVG) 
   .then(optimize) 
   .then(svgToMiniDataURI) 
   .then(encodeSpaces)

So we'd just use this instead?:

 const svgToMiniDataURI = require(`mini-svg-data-uri`).toSrcSet

[tigt]

Yep, that’s exactly what I had in mind.

If you’d like this change to happen ASAP I’d recommend a PR, but if it’s not that urgent I’ll probably get to it this weekend.

---

(…is that default arg really called turdSize?)

[polarathene]

We're in no rush. I'm flat out with other projects atm, so I won't have the time to contribute here sorry. This is a really great project though, I wasn't aware of what this library was doing until just recently!

(…is that default arg really called turdSize?)

Yep..

-t, --turdsize n - suppress speckles of up to this size (default 2)

Leads to some fun issue names like determine optimal turd size for potrace.

[tigt]

I did the simplest implementation that could possibly work in #11 — would you mind seeing if that branch works for your code, please?

[polarathene]

@tigt Hey sorry about the delay. Just got around to testing that out and it works great! :smiley:

Well, I had to make a slight tweak as it seems the current code is causing problems with your this reference:

The GraphQL query from /storage/projects/gatsby_test/using-gatsby-image/src/pages/traced-svg.js failed.

Errors:
  this is not a function

  GraphQL request (25:3)
  24: fragment GatsbyImageSharpFixed_tracedSVG on ImageSharpFixed {
  25:   tracedSVG
        ^
  26:   width
  ,this is not a function

Where tracedSVGcontains a call to your toSrcset() method. This is from just passing the function to a .then chained to a promise. If you're unfamiliar with this issues, here's some links:

https://stackoverflow.com/questions/37334284/javascript-promises-this https://stackoverflow.com/a/20279485 https://stackoverflow.com/a/4700899

Which afaik in this situation would seem you should use a closure by moving the srcSet method into the main function body and setting it there as a property?(haven't really defined a function before where a function property is added to it that calls itself)

I ended up making this small change, swapping the this for the main functions name instead, it appears to work properly now:

svgToTinyDataUri.toSrcset = function toSrcset(svgString) {
  return svgToTinyDataUri(svgString).replace(/ /g, '%20');
}

[tigt]

Man, 4 years in JS and I still screw up this. I’ll do what you did.

[polarathene]

Hey @tigt , any chance of making that change and merging the PR? :)

[tigt]

Whoops, I completely goofed. I’ll publish a new version tonight.

[tigt]

Should be up on npm as v1.1.3. Thank you for your patience and help!