fix: don't default to 0 (options.limit)
#74
Conversation
alexander-akait
force-pushed
the
issue-40
branch
2 times, most recently
from
de90968
to
2b0529b
Compare
alexander-akait
changed the title
limit options now works correctly.
|
@michael-ciniawsky I'll just do rebase after |
|
/cc @michael-ciniawsky can we merge this before |
|
If it's beneficial to the current major version range, land it before defaults so consumers can benefit from it without having to upgrade. |
index.js
Outdated
| var limit = (this.options && this.options.url && this.options.url.dataUrlLimit) || 0; | ||
| if(query.limit) { | ||
| limit = parseInt(query.limit, 10); | ||
| var options = Object.assign({}, loaderUtils.getOptions(this)); |
There was a problem hiding this comment.
loaderUtils.getOptions(this) || {};
Object.assign is slower and there is nothing (e.g defaults) to assign
There was a problem hiding this comment.
FYI - It's slower by quite a bit in some cases. I initially tried doing this when we had to rush through the loaderUtils deprecation updates & you take a real performance hit.
index.js
Outdated
| limit = parseInt(query.limit, 10); | ||
| var options = Object.assign({}, loaderUtils.getOptions(this)); | ||
| // Options `dataUrlLimit` is backward compatibility with first loader versions | ||
| var limit = options.limit || (this.options && this.options.url && this.options.url.dataUrlLimit); |
index.js
Outdated
| } | ||
| var mimetype = query.mimetype || query.minetype || mime.lookup(this.resourcePath); | ||
| if(limit <= 0 || content.length < limit) { | ||
| var mimetype = options.mimetype || options.minetype || mime.lookup(this.resourcePath); |
index.js
Outdated
| var mimetype = query.mimetype || query.minetype || mime.lookup(this.resourcePath); | ||
| if(limit <= 0 || content.length < limit) { | ||
| var mimetype = options.mimetype || options.minetype || mime.lookup(this.resourcePath); | ||
| // No limits or limit more than content length |
index.js
Outdated
| } | ||
|
|
||
| var fileLoader = require("file-loader"); | ||
| return fileLoader.call(this, content); | ||
| } |
index.js
Outdated
| @@ -6,17 +6,19 @@ var loaderUtils = require("loader-utils"); | |||
| var mime = require("mime"); | |||
| module.exports = function(content) { | |||
michael-ciniawsky
changed the title
options.limit)
michael-ciniawsky
changed the title
options.limit)0 (options.limit)
alexander-akait
force-pushed
the
issue-40
branch
6 times, most recently
from
8ceccf9
to
48dd8a0
Compare
|
/cc @michael-ciniawsky done also remove |
| module.exports = function(content) { | ||
| this.cacheable && this.cacheable(); |
There was a problem hiding this comment.
Can we really drop it yet ? I'm not against it, but webpack =< v1.0.0 users will lose caching then.
There was a problem hiding this comment.
That's a great point & a good catch, we can't as this technically still supports 1.x. @evilebottnawi that will have to stay in and be removed in the defaults PR.
alexander-akait
force-pushed
the
issue-40
branch
from
48dd8a0
to
fc91e3c
Compare
|
/cc @d3viant0ne |
What kind of change does this PR introduce?
Refactoring
Did you add tests for your changes?
manual testing 😄
If relevant, did you update the README?
not required
Summary
Consistent getting
limitoptions, also don't use 0 as defaultlimitvalue because this is misleading when reading the code: if nolimitencode all, iflimitenable encode only files smaller thanlimit.Does this PR introduce a breaking change?
No
Other information
not required