-
Notifications
You must be signed in to change notification settings - Fork 3.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Limit data-uri size to 32KB, falling back to relative url() #1190
Conversation
Apparently there is some disagreement on the tangent of 40 between browser and node. 42 is the answer.
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.
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)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use tree.Quoted instead of tree.Anonymous then the url(
will be quoted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, I'll push that shortly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
'compress', // whether to compress | ||
'ieCompat', // whether to enforce IE compatibility (IE8 data-uri) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realized ieCompat
should be in the evalEnv
, not parseEnv
.
regarding Quote - if you keep it anonymous and the path contains '"() then this will break because they are not escaped. |
Ah, yes, fair enough. Update incoming. |
@agatronic Any chance we can merge this in soon? Let me know if you have any remaining issues. |
@evocateur done. Note: I had to change it to make the tests pass with both types of seperators. |
Great, thanks! |
Version 8 of every web dev's favourite browser limits
data-uri
s to 32KB. Since we're now in the business of embeddingdata-uri
s, we should avoid exceeding this.A new option for
lessc
has been added,--no-ie-compat
.By default, the corresponding env property
ieCompat
istrue
. Passing--no-ie-compat
or settingenv.ieCompat
tofalse
will avoid returning the fallbackurl()
and always embed thedata-uri
.The fix to
tan()
was just something random I saw while trying to make bothnpm test
andmake browser-test
pass cleanly. I have no idea whynode
andphantomjs
would decide those products differ (only the last significant decimal was off by one in PhantomJS), buttan(42)
fixed it.