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

fix: don't default to 0 (options.limit) #74

Merged
merged 1 commit into from
Jun 12, 2017

Conversation

alexander-akait
Copy link
Member

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 limit options, also don't use 0 as default limit value because this is misleading when reading the code: if no limit encode all, if limit enable encode only files smaller than limit.

Does this PR introduce a breaking change?

No

Other information

not required

@alexander-akait alexander-akait changed the title Fixed: limit options now works correctly. Chore: refactoring Apr 20, 2017
Copy link
Member

@michael-ciniawsky michael-ciniawsky left a comment

Choose a reason for hiding this comment

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

But I have the webpack-defaults update ready for this loader maybe wait and fix it there instead, give me a few moments please, it's mainly blocked bc of schema-utils update :)

@alexander-akait
Copy link
Member Author

@michael-ciniawsky I'll just do rebase after webpack-defaults 😄

@michael-ciniawsky michael-ciniawsky changed the title Chore: refactoring chore: refactoring Apr 20, 2017
@alexander-akait
Copy link
Member Author

/cc @michael-ciniawsky can we merge this before webpack-defaults was applied?

@joshwiens
Copy link
Member

joshwiens commented Jun 9, 2017

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));
Copy link
Member

Choose a reason for hiding this comment

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

loaderUtils.getOptions(this) || {};

Object.assign is slower and there is nothing (e.g defaults) to assign

Copy link
Member

Choose a reason for hiding this comment

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

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);
Copy link
Member

Choose a reason for hiding this comment

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

\n

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);
Copy link
Member

Choose a reason for hiding this comment

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

\n

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
Copy link
Member

Choose a reason for hiding this comment

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

\n

index.js Outdated
}

var fileLoader = require("file-loader");
return fileLoader.call(this, content);
}
Copy link
Member

Choose a reason for hiding this comment

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

\n

index.js Outdated
@@ -6,17 +6,19 @@ var loaderUtils = require("loader-utils");
var mime = require("mime");
module.exports = function(content) {
Copy link
Member

Choose a reason for hiding this comment

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

\n

@michael-ciniawsky michael-ciniawsky changed the title chore: refactoring fix: don't default to 0 (options.limit) Jun 9, 2017
@michael-ciniawsky michael-ciniawsky changed the title fix: don't default to 0 (options.limit) fix: don't default to 0 (options.limit) Jun 9, 2017
@alexander-akait alexander-akait force-pushed the issue-40 branch 6 times, most recently from 8ceccf9 to 48dd8a0 Compare June 10, 2017 13:35
@alexander-akait
Copy link
Member Author

/cc @michael-ciniawsky done

also remove this.cacheable && this.cacheable();

module.exports = function(content) {
this.cacheable && this.cacheable();
Copy link
Member

@michael-ciniawsky michael-ciniawsky Jun 10, 2017

Choose a reason for hiding this comment

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

Can we really drop it yet ? I'm not against it, but webpack =< v1.0.0 users will lose caching then.

Copy link
Member

Choose a reason for hiding this comment

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

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.

@michael-ciniawsky michael-ciniawsky added this to the 0.5.9 milestone Jun 10, 2017
@alexander-akait
Copy link
Member Author

/cc @d3viant0ne

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants