Skip to content
This repository has been archived by the owner on Mar 17, 2021. It is now read-only.

feat: add encoding & generator options #210

Merged
merged 15 commits into from Apr 8, 2020

Conversation

EslamHiko
Copy link
Contributor

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

Motivation / Use-Case

Fix: #25

Breaking Changes

No

Additional Info

src/index.js Outdated Show resolved Hide resolved
Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One note

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work, one note - original issue was for supporting something line this https://github.com/bhovhannes/svg-url-loader (allow to reduce size of svg), I don't know beset solution, maybe we should to add a new option what allow to implement own custom source.toString(), maybe the source option and provide example with https://github.com/tigt/mini-svg-data-uri

@EslamHiko
Copy link
Contributor Author

@evilebottnawi I don't think source as a name would make much sense about what it does what do you think about changing it to sourceEncoding or contentEncoding.

package.json Outdated Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated
loader: 'url-loader',
options: {
source: (svgContentBuffer) => {
return `data:image/svg;utf8,${svgContentBuffer.toString(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add example with mini-svg-data-uri package

Copy link
Contributor Author

@EslamHiko EslamHiko Apr 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@evilebottnawi I've added an example here: https://github.com/webpack-contrib/url-loader/pull/210/files/2bb2de67e75d175c1ab7b41a075307414f8b6930#diff-04c6e90faac2675aa89e2176d2eec7d8R259
this example to tell the people what is the final format of the final output to help them develop their own implementation.

src/index.js Outdated
@@ -45,11 +46,13 @@ export default function loader(src) {
const esModule =
typeof options.esModule !== 'undefined' ? options.esModule : true;

const encodedData = options.generator
? options.generator(src)
: `data:${mimetype || ''};${encoding},${src.toString(encoding)}`;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look here https://developer.mozilla.org/en-US/docs/Web/HTTP/Basics_of_HTTP/Data_URIs

encoding is optinal, so

data:text/html,%3Ch1%3EHello%2C%20World!%3C%2Fh1%3E is valid, but in our case we always insert ;

Rewrite it to "data:${mimetype || ''}${encoding? ;${encoding} : ''},${src.toString(encoding)}`;" + test

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added boolean type for encoding, now we can disable it, if it's enabled it will use the default base64

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One note

@@ -48,7 +52,9 @@ export default function loader(src) {

const encodedData = options.generator
? options.generator(src)
: `data:${mimetype || ''};${encoding},${src.toString(encoding)}`;
: `data:${mimetype || ''}${encoding ? `;${encoding}` : ''},${
encoding ? src.toString(encoding) : src.toString()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be simplified to src.toString(encoding), when encoding is undefined it will be as src.toString()

Copy link
Contributor Author

@EslamHiko EslamHiko Apr 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

encoding here is never undefined it's base64 if it was already undefined or true or a string value or false and src.toString(false) will cause a bug.

src/options.json Outdated
@@ -7,7 +7,7 @@
},
"encoding": {
"description": "Specify the encoding which the file will be in-lined with.",
"type": "string",
"type": ["boolean", "string"],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oneOf - https://ajv.js.org/keywords.html#oneof

"oneOf": [ { "enum" : ["value", "value", "value"] }, { "type": "boolean" } ]

@EslamHiko EslamHiko changed the title feat: add encoding option feat: add encoding & generator options Apr 7, 2020
@alexander-akait
Copy link
Member

Thanks!

@alexander-akait alexander-akait merged commit 36f1354 into webpack-contrib:master Apr 8, 2020
@EslamHiko EslamHiko deleted the encoding branch April 8, 2020 11:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add ability to set encodings
2 participants