feat(index): add option to disable base64 encoding (options.base64)

[joekrill]

• Add a base64 option flag (enabled by default)
• Updates the README.md to reflect the new option
• Fixes an additional bug where the semicolon was only added when a
  matching mime type was found -- but the semicolon should always be
  present when base64 encoding is used, regardless of whether a mime
  type is specified (see https://tools.ietf.org/html/rfc2397)

resolves #25

[brettvitaz]

It seems to me that "url?-base64" is not conventional and will cause confusion. I believe that it would be better to be more explicit.
e.g. "url?encoding=none" is more understandable and will allow for specifying alternate encodings, if necessary.

[joekrill]

@brettvitaz I wasn't a huge fan of that syntax either, but my intention was actually to follow convention. Webpack's loader-utils says that this is how a flag is toggled using query parameters (see the parseQuery documentation).

And RFC2397 really treats this as a flag, which is why I went in that direction. I think adding additional encodings to the spec is highly unlikely. And I didn't want to add needless complications to the code. If instead we used a parameter that takes a variable value, then there would need to be some additional error checking done. What should be done if an incorrect or blank value is provided? Fallback to the default? Throw an error? Do we ignore case? So it seemed to me best to just go with a toggle and avoid that -- there's no real upside to it, as far as I could tell. But if the consensus is that this should be a more explicit parameter, I'm happy to implement it that way.

[SpaceK33z]

Do you know if it's possible to automatically detect base64? That would be better than manually having to specify it. I haven't properly looked into this issue yet so don't know if it's possible for sure.

Btw, in a few weeks I'm going to spend some time on file-loader and url-loader, and I'll come back to review this PR :).

[joekrill]

@SpaceK33z I suspect it may be possible to analyze the buffer and determine if every octet is an ASCII character, and if so then don't bother using base64 encoding. I'm not sure how reliable that would be.

[michael-ciniawsky]

@joekrill We need to land #70 beforehand, but after that I would like to add this if you're still interested :)

[michael-ciniawsky]

In any case you need to sign the CLA by closing and reopening the PR to trigger the CLA Bot again :)

[joekrill]

@michael-ciniawsky Yeah not a problem -- I'll keep en eye on #70 and submit a new PR when that's merged, if that seems reasonable? Should I close this PR then?

[michael-ciniawsky]

@joekrill You can leave it open for reference, give me 1-2 days to finish #70, depending on other stuff I have on my plate 😛 I come back here when done

[michael-ciniawsky]

I intend to add an encoding option (options.encoding) within #70, but I need to check it again (it's been a while), I come back here soon and we can 'backport' it before #70 (v1.0.0) lands :)

[michael-ciniawsky]

@joekrill Mind to elaborate on encodeURIComponent in #79 :) ? You can also update the PR here with this changes, but the feature (option) should be more generic then 'just' disabling base64 encoding

[joekrill]

@michael-ciniawsky It's been a while so I'm trying to refresh my memory about this, so hopefully I'm not missing anything. Here's the relevant part from the RFC regarding data URLs:

The appearance of ";base64" means that the data
is encoded as base64. Without ";base64", the data (as a sequence of
octets) is represented using ASCII encoding for octets inside the
range of safe URL characters and using the standard %xx hex encoding
of URLs for octets outside that range.

So given that, my takeaway was/is:

1. This is simply a toggle -- either it is base64 encoded, or it isn't. There's nothing in here about the possibility of other encodings, and it seems highly unlikely that's going to change anytime soon (this spec is almost 20 years old at this point, and as far as I can tell hasn't been superseded). So, to me, making this more generic just seemed like overkill. Not only that, but it then makes the configuration more confusing. Users may think they can specify other encodings here when the only encoding that is actually valid is base64.

2. That quote also indicates that without base64 encoding non-safe URL characters need to be hex-encoded. So that's why I used encodeURIComponent, and I think that's going to be necessary in feat: add encoding option (options.encoding) #79, as well. This aspect also makes it a bit more problematic if we use a more generic encoding option instead of simply a toggle, because then the question will be -- when do we use encodeURIComponent? Only when encoding is blank or false? Or do we use it when the value anything but base64? And there's no guidance in the spec around that -- particularly because there's no other encoding options in the spec, so there's no need.

Also I noticed in #70 that the conditional addition of the semicolon is incorrect. In the code it's included only if there is a mime type value. But it should actually be dependent on whether the base64 token is present, not on whether the mime type is present. Currently the code will produce this if there is no mime type:

data:base64,<data>

And the "base64" string would actually be interpreted as the mime type. It should actually be:

data:;base64,<data>

Again, I'm basing this entirely on my interpretation of the RFC. As far as I can tell 2397 is the relevant one, but they can get a bit confusing so perhaps there's a different spec we should be following?

If you want me to comment on #70 instead, let me know. Or if you want me to do some work on this, I'm happy to.

[michael-ciniawsky]

@joekrill first, sry for the delay :) && thx for the explanation

• For feat: add encoding option (options.encoding) #79 if you like to finish that here, we better just close it :D, my intension was more of supporting the https://www.npmjs.com/package/svg-url-loader usecase in url-loader aswell, but maybe this needs to be implemented in a different way (based on MIME type) and is unrelated to encoding in particular

• refactor: add webpack-defaults #70 needs to be rebased, tested && will be webpack >= v2.0.0 only as a semver major, it definitely comes after options.encoding landed in one way or the other 😛

options.encoding

{
   loader: 'url-loader'
   options: {
      encoding: false // (default: true) {Boolean} 
   }
}

?

[shellscape]

@joekrill resurrecting this one from the dead as the repo is getting some love. Are you interested in brining this PR up to date with the current codebase and continuing review?

[ooflorent]

It would be great if the option could be set from options. I think this change needs a test. Could you add one please?

[michael-ciniawsky]

This needs to be implemented as a loader option instead of a sole query param

[michael-ciniawsky]

This PR is definitely stalled. I'm going to close it (temporarily) within the next ~48-72 hours due to that fact. Feel free to reopen at any time

[joekrill]

Sorry everyone, I'm trying to get myself back up to speed on this and the changes to Webpack loaders since I first made this PR. But it looks like it should be pretty straightforward to update. Let me see if I can get this updated quickly.

[jsf-clabot]

All committers have signed the CLA.

[joekrill]

@michael-ciniawsky @ooflorent rebased and updated this as requested.

[michael-ciniawsky]

Bad rebase as it seems :) This file should be obsolete

[michael-ciniawsky]

Why 'binary' (latin1) here? Wouldn't 'utf8' be 'better'?

[joekrill]

'binary' is required for this to work with binary images (PNG, JPEG, etc)... which, while a bit unusual, seems to be perfectly valid.

[michael-ciniawsky]

Please test with an actual .svg file here. Create a base64 folder in test/fixtures with the following contents (you can copy file.svg => base64/file.svg)

fixtures
|– base64
||– file.svg
||– fixture.js // import svg from './file.svg' ...
|– ...
|–fixture.js 

and

-  const stats = await webpack('fixture.js', config);
+  const stats = await webpack('base64/fixtures.js', config);

[alexander-akait]

Closing due to inactivity. Feel free to reopen new PR. Thanks!