Limit data-uri size to 32KB, falling back to relative url() #1190

Closed
wants to merge 7 commits into
from

Conversation

Projects
None yet
2 participants
Contributor

evocateur commented Feb 19, 2013

Version 8 of every web dev's favourite browser limits data-uris to 32KB. Since we're now in the business of embedding data-uris, we should avoid exceeding this.

A new option for lessc has been added, --no-ie-compat.

By default, the corresponding env property ieCompat is true. Passing --no-ie-compat or setting env.ieCompat to false will avoid returning the fallback url() and always embed the data-uri.

The fix to tan() was just something random I saw while trying to make both npm test and make browser-test pass cleanly. I have no idea why node and phantomjs would decide those products differ (only the last significant decimal was off by one in PhantomJS), but tan(42) fixed it.

evocateur added some commits Feb 19, 2013

Change tan function test value.
Apparently there is some disagreement on the tangent of 40 between browser and node. 42 is the answer.
Prevent data-uri function from embedding files larger than 32KB.
Although IE8 does support data-uris, it only does so with a limit of 32KB. It's a silly limitation, but a source of potential bugs. When the limit is exceeded, the data-uri() function will simply return a normal url() value with a relative path to the asset.

One may pass --no-ie-compat to lessc to avoid this safeguard.
lib/less/functions.js
+ if (this.env.ieCompat !== false) {
+ // TODO: respect verbose or silent flags here
+ console.error("Skipped data-uri embedding of %s because its size (%dKB) exceeds IE8-safe %dKB!", filePath, fileSizeInKB, DATA_URI_MAX_KB);
+ return new(tree.URL)(new(tree.Anonymous)(filePath));
@lukeapage

lukeapage Feb 19, 2013

Owner

use tree.Quoted instead of tree.Anonymous then the url( will be quoted

@evocateur

evocateur Feb 19, 2013

Contributor

Makes sense, I'll push that shortly.

@evocateur

evocateur Feb 19, 2013

Contributor

On the other hand, quoting isn't necessary inside url() statements. I'm not sure why the browser version inserts the quotes, but neither syntax is breaking anything except conformity of output between the browser and CLI versions.

lib/less/functions.js
+
+ if (this.env.ieCompat !== false) {
+ // TODO: respect verbose or silent flags here
+ console.error("Skipped data-uri embedding of %s because its size (%dKB) exceeds IE8-safe %dKB!", filePath, fileSizeInKB, DATA_URI_MAX_KB);
@lukeapage

lukeapage Feb 19, 2013

Owner

not sure about console.error - is this an error or a warning or just info?

whats our plan with supporting verbose/silent flags? Guess they just need putting in the env from the options?

@evocateur

evocateur Feb 19, 2013

Contributor

Semantically, you're right, this is not strictly an error. A warning is more appropriate here. However, in node, console.warn and console.error both emit to stderr, so the difference is academic.

I tried to think of a different place to put the verbose and silent flags, but nothing makes as much sense as the env object (I get nervous about bloating it too much, but then realize it's just a lightweight object with a series of properties...). I like the approach of a utility logging function, similar to less.writeError in lib/less/index.js. It would allow a central point of config evaluation, formatting, etc. However, that pattern would involve a lot more monkeying around with scope and whatnot, since functions.js does not have access to the less object, nor is there currently a concept of "global" config flags (instead, they're copied and passed around).

For now, adding verbose and silent to the env flags will do. I'll patch it later today.

@lukeapage

lukeapage Feb 19, 2013

Owner

long term I would probably add a log function to env - that's why I made it an object is to provide the glue methods that need to be done in multiple places.. but I don't mind if you just add those 2 properties for now.

'compress', // whether to compress
+ 'ieCompat', // whether to enforce IE compatibility (IE8 data-uri)
@evocateur

evocateur Feb 19, 2013

Contributor

I realized ieCompat should be in the evalEnv, not parseEnv.

+
+ if (this.env.ieCompat !== false) {
+ if (!this.env.silent) {
+ console.warn("Skipped data-uri embedding of %s because its size (%dKB) exceeds IE8-safe %dKB!", filePath, fileSizeInKB, DATA_URI_MAX_KB);
@evocateur

evocateur Feb 19, 2013

Contributor

I ended up "lowering" this message to a warn, wrapped in the silent check. Definitely agree an env.log function is the way to go in the future.

Owner

lukeapage commented Feb 19, 2013

regarding Quote - if you keep it anonymous and the path contains '"() then this will break because they are not escaped.

Contributor

evocateur commented Feb 19, 2013

Ah, yes, fair enough. Update incoming.

Contributor

evocateur commented Feb 26, 2013

@agatronic Any chance we can merge this in soon? Let me know if you have any remaining issues.

@lukeapage lukeapage closed this Feb 26, 2013

Owner

lukeapage commented Feb 26, 2013

@evocateur done.

Note: I had to change it to make the tests pass with both types of seperators.

@lukeapage lukeapage reopened this Feb 26, 2013

@lukeapage lukeapage closed this Feb 26, 2013

Contributor

evocateur commented Feb 26, 2013

Great, thanks!

@evocateur evocateur deleted the evocateur:data-uri-size branch Feb 26, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment