Skip to content
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

Add a data-uri function. #1086

Closed
wants to merge 3 commits into from
Closed

Add a data-uri function. #1086

wants to merge 3 commits into from

Conversation

jneen
Copy link
Contributor

@jneen jneen commented Dec 23, 2012

Useful for adding in data uris from files.

Usage:

data-uri(@mimetype, @filepath)

will render

uri(data:some/mimetype,filecontents)

If the mimetype has the ;base64 addition, the file will be base64 encoded.

Enjoy!

as described in a comment on less#775
@jneen
Copy link
Contributor Author

jneen commented Dec 27, 2012

Hm. Is this possibly problematic because it requires node's fs module? I see that less is supposed to be supported browser-side too. One solution would be to require('fs') inside the implementation...? But this function really doesn't make any sense in the browser, does it? In the browser, maybe a reasonable thing to do would be to simply return url(@filepath) and hope that the path checks out?

@lukeapage
Copy link
Member

it doesn't make much sense browser-side to me, so returning uel(@filepath) sounds like a good idea.

@jneen
Copy link
Contributor Author

jneen commented Dec 27, 2012

Cool, I'll try that. Do you have some kind of isBrowser() function, or should this function test for it itself? The other option is just to straight-up throw a parse error if require('fs') fails.

@lukeapage
Copy link
Member

either that or typeof window are the way it is done at the moment.

@lukeapage
Copy link
Member

Possible enhancement... Could it default to trying to work out the mime type? If you only get one argument...

@jneen
Copy link
Contributor Author

jneen commented Dec 28, 2012

Hm, what's the best way to work out the mimetype? File extension, maybe? That could be some pretty complicated logic...

@jgallen23
Copy link

https://npmjs.org/package/mime should work

@lukeapage
Copy link
Member

The previous pull used file extension with one special case for jpg.. I
wondered if this might cover most use cases?

Could use a node package, but would rather be cautious about adding a new
dependency.

and move require('fs') into the function, so other things
still work in the browser.
@jneen
Copy link
Contributor Author

jneen commented Dec 28, 2012

I ended up adding the dependency for mime. The previous PR used image/#{ext}, but this is a bit more general than just images - I have a use case for inlining a font, for example.

@lukeapage
Copy link
Member

pulled into 1.4.0

Also fixed the browser to return url and moved tests to url.less - that way it isn't executed in the browser tests.

@lukeapage lukeapage closed this Jan 4, 2013
@jneen
Copy link
Contributor Author

jneen commented Jan 5, 2013

Neat, thanks!

@matthew-dean
Copy link
Member

There were some things missed in this pull request and problems introduced with this code which need to be addressed.

a) Less.js already has properties set up which return whether or not the parser is set to a browser, Node, or Rhino environment. However, this code ignores all of this and attempts to again detect browser support, even though browser support has already been detected in a way that can be overridden. So, this code breaks configurability.

b) Up until this pull request, file operations were restricted to @import statements. The importer in Less.js is designed so that a JavaScript author can adapt the plugin to their environment by overriding the less.Parser.importer function in order to handle file operations. This code breaks that modularity as well by assuming and only supporting Node-based operations, which means that file operations are no longer supported consistently and universally. A JavaScript author can override and support the importer for file operations in unique environments, but not data-uri. (See Issue #1392)

Neither of these things are well-documented, so it's understandable that they were missed, but they should be addressed moving forward (preferably in a bug-fix release).

We're exploring making a more graceful environment plugin model that is more environment-agnostic, with a more clearly-defined API, so that we can write features like this without having to make explicit code paths for Node, Rhino and/or the browser. But for now, data-uri should be adjusted to match the current model for importers.

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

Successfully merging this pull request may close these issues.

4 participants