Support importing from npm packages #1972
Conversation
|
I'm not sure how to go about adding tests to this, but all existing tests seem to pass locally. |
|
Yes, please this is awesome! |
|
+1 |
|
This would be nice! +1 |
|
As cool as this is, it opens a can of worms by setting the precedent of the library supporting package managers. Does it also have to support bower, and whatever other manager people can think of? |
|
npm is the package manager you already use to get less, so it kinda makes sense to do that one |
|
@juliangruber It is also on bower, nuget, ruby gems, and others as well. |
Given that less.js is already installed via npm, I'm not sure how that conclusion can be drawn.
Actually, besides bower, this project does not directly support the others. We're glad they exist, but they are community maintained. |
Being published on a package manager, and supporting extra api for that manager are very different. Just wanted to point out the possible slippery slope. Thanks all! |
|
Could custom importers be supported as plugins? Anyone could then add any less source type as he likes, be it package manager, database, key/value store, something that generates less on the fly etc. Custom importer does not have to need special import command directives, it can be simply given url string and parse it. At least when it comes to less4j, people seemed to be interested in such customizations. |
|
+1 for doing this via plugin. |
|
agreed, plugins are the way to go! |
|
👍 for a plugin, I think that makes the most sense to me! |
|
+1 I absolutely need this. Plugin would be fine, as it would allow us to do the same for |
|
Can we combine |
|
|
||
| return info; | ||
| }, | ||
| paths: process.env.NODE_PATH ? process.env.NODE_PATH.split(':') : [] |
these are completely separate concepts, I don't think combining them by default is a good idea. But you're underscoring the fact that one might need to either import or reference a npm stylesheet. I'm not sure how that would work. |
|
Yeah, sorry, I didn't mean we should combine them by default. |
|
They can be combined e.g. (npm, reference) when required. I like the idea but I agree it would be better as a plugin. I can look at adapting this pull request after 2 if you don't want to help development of the API to allow this. |
|
As others have said: in general, I think the smartest design philosophy is that the core parsing library be one that can run in any JavaScript management with any package manager (or, as it has been historically, in the browser). But, as a plugin, it makes sense to extend specific environments. And it's another opportunity to build a more robust plugin library. @lukeapage Is there a guide / process in mind for how people would submit pull requests for plugins? |
|
Does anyone have any data on how often less is used in different environments? We could build it for node.js and have it work in all the browsers via browserify. Depends if there are lots of people using this in non-node.js & non-browser environments? |
|
I don't think we have such data. And something else to consider: in some cases, the browser build is used in non-browser, non-Node environments because it's been more "environment-agnostic" than the Node version. |
|
Its seperation of concerns as much as anything. Browserify wouldn't work.. |
|
P.s. re a guide, no I'm still writing a plugin api on the 2 branch. Anyone |
|
The browserified version should work in any JavaScript environment, browser/node/rhino/whatever else. The separation of concerns is not cleanly done here. The coupling between all the different parts of the less parser/compiler is really tight. It would be nice to get clean separation into lexer, parser, code-generator, and file-system (i.e. all IO operations, including web requests). It would require almost a total re-write, but I think would pay off in the long term. This probably isn't the right place for that discussion. I'd be happy to talk more about it, and possibly even do the initial refactoring work, but it's not directly related to adding support for importing less from npm, which I believe is a common enough use case that we should be looking to support it. |
|
@ForbesLindesay see #1902 I am trying to indicate what we are working towards. As you can see it uses browserify. I don't see how using browserify would allow us to build a browser version that can fetch files from npm and even if that was possible I don't think it would be wanted for the core browser version. |
|
#1902 looks exciting. I'll look into that as it seems more relevant. You could use browserify to load files from npm if you used the asynchronous version of resolve and then implemented |
| @@ -82,6 +83,27 @@ less.Parser = function Parser(env) { | |||
| callback(e, root, importedPreviously, fullPath); | |||
| }; | |||
|
|
|||
| if (importOptions.npm) { | |||
| path = resolveNpmFile(path.replace(/\.less$/, ''), { | |||
There was a problem hiding this comment.
@ForbesLindesay mind removing the regex/replace here? thx
YES! |
Agreed! I'm not intending to merge this yet, in case that's the concern. For now, while this PR is outstanding, I just want to continue doing testing directly from @ForbesLindesay's fork. |
|
The replacement is necessary because less adds the extension if the user did not provide an extension. This means that |
It's not though. Less does not require a file extension for less files. Regardless, I've been testing extensively with, without, and every way in between.
It would if the repo's name is
This is just opinion, so let's drop the regex and leave that to users. |
|
I think he's saying that the lib by default requests twbs.less if you only specify just twbs. Which would be true when importing files. Not sure about the npm context. |
currently the regex does a replace on the filepath/module name that's passed in, e.g. |
|
This is now added as a plugin |
|
Awesome! |
|
Note: requires v2 branch.. Though am thinking of merging that to master soon.. |
Will there be a published API for comments? |
|
Which part of the api? I wrote the plugins doc on the plugins issue. I'll |
|
GOOD!!! |
This adds support for importing from npm packages installed to node_modules. For example, having installed
twbsvia npm you can import the entirety of bootstrap using:or a specific file via:
The syntax and resolution algorithm will be familiar to most node.js developers, and is becoming increasingly popular on the client side too with libraries such as browserify.
closes #1965