Fix import path for plugins

[Franziskus1988]

#3181 Less shouldn't search for plugins in process.cwd(), but in same node_modules folder like less is installed.

[seven-phases-max]

This way it won't work of course (you're modified the paths option that does not and should not have any effect on where command line plugins are searched).

[Franziskus1988]

@seven-phases-max

With my fix:

Without my fix:

Have a look at the paths var!

Maybe it should not effect the plugin loading, but it does! And adding process.cwd()/node_modules to paths doesn't make any sense either, because it assumes that your current terminal location is always in a npm installation root dir.

[seven-phases-max]

Maybe it should not effect the plugin loading, but it does!

Well, I understand that it makes it accidentally to work for your particular setup and directory structures (I can read it in the corresponding thread), it just does not fix the problem at all. It's no way even a quick-and-dirty fix because "quick-and-dirty" is assumed to not break anything else (at least major) while your change simply breaks the whole "import" facility.
I'm afraid for real (even quick-and-dirty one) fix you'll need to find the code where Less is actually looking for cli-plugins and make sure it does look a local node_modules directory (where Less is installed) and the global node_modules directory (both using node/npm functions, and not by injecting "../../../../" or whatever hardcoded route which does not make any sense at all, sorry).

It's not hard to guess why paths affects the cli-plugins (plugin loader is modified in 3.x to support @plugin statement), but yet again it should not and this is what most likely this bug is about.
(@plugin plugins should be searched by "import" methods (and then by "node" methods) incl. the "paths" option, cli-plugins should be searched only by "node" methods. 3.x changed both (as I can guess) to use "import" methods only, hence the bug).

[seven-phases-max]

I guess I'll reopen the PR just marking it "consider closing" instead assuming you'll try to find a proper method (you'll update the PR by making new commits to your branch). Closing it instantly was just too harsh maybe.

[Franziskus1988]

It's your good right to close the PR since it was an ugly and not useful attempt 😄 I just saw a comparable approach to get the right folder in the plugin-loader of less 2.7 (require("../../../" + name)). My brain is currently melted from programming in groovy. Sorry!

[matthew-dean]

not by injecting "../../../../" or whatever hardcoded route which does not make any sense at all, sorry

@seven-phases-max Technically, @Franziskus1988 is right. There was a hard-coded value that was removed in the plugin refactor. Unfortunately, we had no test that tested that scenario, so things continued to work without it. See: https://github.com/less/less.js/blob/v2.7.3/lib/less-node/plugin-loader.js#L58

The thing about the pathing in 2.x is that it was kind of magic to plugins, and specifically to lessc plugins, vs. properly reflecting a file management / file lookup strategy that was environment-specific, which is how the file managers / environments are supposed to work. So I refactored that to a) support any kind of declared plugins, b) be at a core file management level. BUT, that probably meant that I broke something for a specific scenario that wasn't tested.

What happens now is that the Node environment adds lookup paths to the less default options. See: https://github.com/less/less.js/blob/master/lib/less-node/index.js#L20

By the looks of it, that means that this approach that @Franziskus1988 is largely correct, although I wouldn't delete and replace path.join(process.cwd(), "node_modules"), but add to the lookup paths array for paths in that location.

BUT: @Franziskus1988 if you want a proper PR to be considered, that would mean you need to then do the following:

1. Don't remove existing lookup paths (unless you can prove why the existing one is invalid). You're likely going to break something.
2. Run tests and update all the "error" scenarios which list out every path tried when they try to retrieve a file. The error message tests need to work for Node and continue to work for the browser.
3. Document in much greater detail (in the code), why this magic ../../../ actually works. What is the lookup path that's being considered, and why is a specific parent folder, and only that specific parent folder X levels up the correct one every time? I know that wasn't done the first time, which was essentially why it was removed. The documentation said, "is at the same level as the less.js module". Well how does ../../../ equal "the same level"? Intuitively, "the same level" is ./. So what exactly is/was happening there?
4. If possible, add a test that breaks and then passes with this code put back in.

I'm fine with a "magic value" if it can be PROVEN via documentation that the magic value is legitimately correct. I don't see that yet. I just see, "This makes this work now for me," which isn't quite good enough to be merged.

If you can do that, a fix would be great!

[matthew-dean]

@plugin plugins should be searched by "import" methods (and then by "node" methods) incl. the "paths" option, cli-plugins should be searched by "node" methods. 3.x changed both (as I can guess) to use "import" methods only, hence the bug

@seven-phases-max It's more the reverse. Imports were using the local environment settings, which mapped to the local file manager, which with less-node is specific to a Node environment. Plugins were.... doing something else entirely. Essentially, the previous plugin manager had ANOTHER layer of resolving files outside of the environment/file manager. It was kind of like:

2.x

lessc plugins
lessc file resolver -> less-node plugin file resolver -> less-node file manager -> node module evaluator

@plugin plugins
less-node @import resolver -> less-node plugin file resolver -> less-node file manager -> custom plugin evaluator

@imports
less-node @import resolver -> less-node file manager -> Less @import evaluator

3.x

lessc plugins
less-node file manager -> Less plugin evaluator

@plugin plugins
less-node @import resolver -> less-node file manager -> Less plugin evaluator

@imports
less-node @import resolver -> less-node file manager -> Less @import evaluator

I think I have that documented correctly, but it's been a little while.

In 3.x, there is no special file manager layer for plugins with @plugin syntax. Like any other import, a @plugin says "I need a file with this path identifier", and the local environment tries to find it. There is one slight difference for backwards compatibility, which is that plugin paths will try prepending less-plugin- for short module names. (Like if you used @plugin "my-plugin" or lessc --my-plugin=, it will try to look for both my-plugin and less-plugin-my-plugin. @imports do not do this.)

But other than that, any file requests to the less-node environment are handled identically, and handled the same on Node.js and the browser. That is, the environment, Node or browser, is responsible for resolving files based on the path requested. Plugins should not have additional file path resolving logic outside of the environment's file manager.

[matthew-dean]

Just as an FYI, Less "default options" were also normalized for the Node environment across the API and lessc. Previously, lessc had (slightly) different behavior / options. There actually were no "default options" for an environment. Less core just sort of guessed / assumed. So when I changed the defaults for not evaluating JavaScript inline by default, I normalized options for each environment as well. So, in general, yes, lessc works more like the actual Less API for Node.js. Node === Node now, and it sort of did before but there were some differences.

[seven-phases-max]

Honestly for me https://github.com/less/less.js/blob/master/lib/less-node/index.js#L20
is still making no sense.

1. If I run ../../feek/peek/bin/lessc foo/bar/baz/src.less and this src.less has some @import "other.less" I can't see why it should look for other.less in the totally unrelated (neither to Less installation path nor to src.less path) dirs: <cwd>/node_modules or <cwd>/../../../../node_modules (they would not even point to a valid directories at all).
2. Same way I can't see why for whatever cwd/paths used for lessc --clean-css - the paths option should have any effect at all.

The paths option should have one and the only one meaning:
http://lesscss.org/usage/#less-options-include-paths (applying both for @import and @plugin).
And the option should be empty by default. That is, if some code some where requires some additional cwd, node-modules, whatever lookup - that should be part of this particular some code w/o polluting the option value itself. If it's not - there's something very very wrong happening there.

Well, I guess it would be easier for one of us just to fix it the way we see it right rather than trying to explain how it supposed to be (honestly for me all this "less-node import resolver -> less-node plugin file resolver -> less-node file manager -> custom plugin evaluator" is like a Chinese - I usually think of things by documented options and concrete use-case examples (how it's implemented behind the scenes is not that really important)).

[matthew-dean]

If I run ../../feek/peek/bin/lessc foo/bar/baz/src.less and this src.less has some @import "other.less" I can't see why it should look for other.less in the totally unrelated (neither to Less installation path nor to src.less path) dirs: /node_modules or /../../../../node_modules (they would not point to a valid directories at all).

The file manager resolves file lookups for the environment. In Node, file lookups are local to the file, then use Node's file resolution mechanism. You seem to be arguing that this shouldn't be the case, but it has been the case since @plugin was added. It was just added in a way that wasn't consistent with the file manager pattern, and, as well, it wasn't aligned with lessc's import of plugins.

As far as being valid directories, if you're in your /website directory and run a build script that imports NPM-installed plugins or files, then <cwd>/node_modules will usually be correct, since build scripts are run from the root.

And the option should be empty by default. That is, if some code some where requires some additional cwd, node-modules, whatever lookup - that should be part of this particular some code w/o polluting the option value itself. If it's not - there's something very very wrong happening there.

It doesn't pollute the option.paths value in the way you're thinking. These are default options, which get merged with user options for final options values. The reason it's this way is that the (final) options object gets passed around and tested in various places. Separating / hard-coding an invisible options object that is always separate from the user options object would have involved way more refactoring than this. The user can specify any import paths they want, and those will get merged (joined) into the internal lookup paths for the less-node file manager.

It's possible you could do a different approach which merged hidden paths in the less-node file manager, but either way, in order to have Node module resolution, you're going to have an existing array of lookup paths, which will get added to by the user's paths. It doesn't matter where you put this merge, it's going to happen at some point before file lookups happen. In my opinion, the better way to reason about it was to say, "What are the final set of options Less is going to work with?" and make that clearer in the initial "set up" of the environment.

The documentation you refer to is still accurate; it's just that it was never updated to also refer to @plugin, which essentially is just a special form of @import.

honestly for me all this "less-node import resolver -> less-node plugin file resolver -> less-node file manager -> custom plugin evaluator" is like a Chinese

True, all I was trying to say was:

1. Less, from 2.x, had a straightforward Environment architecture, with a dedicated FileManager (that inherited from the core AbstractFileManager) for both less-browser and less-node that handled file requests specific to that environment.
2. The @plugin PR, as it was accepted at the time, did not integrate or follow this file resolution pattern. Instead, it had loads of it's own file resolution logic, and plugin evaluation logic, which meant that the behavior, context, and default options for plugins (between lessc and @plugin) were wildly different. If you recall, the two were so completely different in how they were coded and evaluated, that we considered actually naming them different things. Instead, we went the direction of aligning as many of these distinct behaviors so that a) plugins behaved more uniformly, b) files were retrieved more uniformly per environment.

There are trade-offs to #2, but the trade-offs seemed less significant than having "plugins" and "modules" or some other confusing multiple-plugin-type system for Less, because that just seemed overkill. It still was a shit-ton of work. Bridging completely different approaches was not easy.

But anyway, hope that clarifies things.

Well, I guess it would be easier for one of us just to fix it the way we see it right rather than trying to explain how it supposed to be

You're not going to offend me if you change things. As far as I'm concerned, in open source, decisions are made by those who show up. Whoever has the time to do the work should do the work. If you have a better way, do a PR and I'll probably give it 1,000 thumbs up. As you know, I'm trying to reduce my time on projects like Less, so whomever wants to do the work, go for it.

[seven-phases-max]

if you're in your /website directory and run a build script that imports NPM-installed plugins or files, then /node_modules will usually be correct,

But I am not (Why should I be?). That's the point. The cwd is allowed to be anywhere and this should not affect any path resolution (neither imports nor plugins) in either of ../../feek/peek/bin/lessc --clean-css foo/bar/baz/src.less or @plugin "foo";.
For the command line plugins the looking path should be related to Less installation path (or more strictly speaking first local then global node_modules dir w/o involving any cwd).
For the @plugin the looking paths should be relative to the less file the statement is in (no cwd yet again) and if not found look at the same Less installation path as above.
And the same for client-side script - @plugin should be resolved relative to current less file and then relative to less.js itself (if possible, since as I recall there were a few issues with this so that sometimes a current html file path is used instead or a sort of (i.e. the only thing that can be interpreted as sort of cwd)).

The cwd directory is the directory only to resolve the command line file paths, e.g. as in ../../feek/peek/bin/lessc foo/bar/baz/src.less both paths are resolved relatively to the cwd (the first one by shell the second one by lessc). Cwd is not for manipulating imports or other language statements.

---

For the rest, yes, I remember @plugin implementation remained to be quite hackish. So I understand how it is/was difficult. So I realize that if proper fix is not possible w/o major rewritting of @plugin then it's possibly more easy to break it back to sort of 2.x state rather than propagating its hacks to the rest of Less code-base.

[matthew-dean]

For the command line plugins the looking path should be related to Less installation path (or more strictly speaking first local then global node_modules dir w/o involving any cwd). For the @plugin the looking paths should be relative to the less file the statement is in (no cwd yet again) and if not found look at the same Less installation path as above.

I don't have the braining today to fully dissect that philosophy, but in general, scripts looking for NPM modules from the root of where you invoked running the script is not really unusual at all. Actually not that unusual in the history of computing and running scripts. 🤷‍♂️

I guess maybe what you're saying is the path should be relative from something more like a path joined with __dirname, which is maybe what ../../../ is supposed to accomplish, so that if you invoke a root script not from that root, it will work. I don't philosophically disagree. The end result should be, yes, the "site" where you installed less, and we may just be saying different things about how to resolve that path.

And I'd also point out again that this isn't code I wrote, but basically code I moved, so that path resolution wasn't happening outside of the file manager, and that plugin resolution wasn't happening outside the core API. The paths chosen were originally here: https://github.com/less/less.js/blob/v2.7.3/lib/less-node/plugin-loader.js#L57-L81

So, philosophically, it sounds like you would argue against one of those path lookups, but you're debating the wrong person. If there's a valid use case where the right answer is derived, then that path should be looked up. At the least, we should support path lookups that were broken with this change, do you agree?

So I realize that if proper fix is not possible w/o major rewritting of @plugin then it's possibly more easy to break it back to sort of 2.x state rather than propagating its hacks to the rest of Less code-base.

Umm... god no. These aren't hacks, in fact the hacks are gone. It's just a list of paths to resolve imports, which includes plugins. It probably just needs one more path to solve this problem. Literally the answer is exactly as I documented it to @Franziskus1988. That is: 1) edit the paths, 2) update tests, 3) document change, 4) submit PR. It's not difficult or complicated at all. It's basically a one-line change + some text edits to error statements + documentation. A few changes to this PR and it would be done.

[seven-phases-max]

: 1) edit the paths, 2) update tests, 3) document change, 4) submit PR.

+100! (I may sound harsh - it's just my "always" because of being the freaking perfectionist, which is not good thing considering the efforts I put into Less recently. So please always consider my comments/remarks as of "A plain Less user" and not a contributor of any kind. @Franziskus1988 notice this please).

So my only requirement for a fix to be a real fix is:
../../feek/peek/bin/lessc --lists foo/bar/baz/src.less thing to work as expected (if it's not then it's not a "fix" of any kind).

[matthew-dean]

@seven-phases-max Based on some more data about the history, does it seem reasonable then that this path would just change to remove process.cwd() and would replace with ../../../../node_modules? The thing that makes me apprehensive is: couldn't a dependency install less in a subfolder? That is, Less can't absolutely be guaranteed to be in node_modules/less AFAIK, and yet it could be part of the dependency tree. So, like you, I feel like this is somewhat a hack, regardless of it being there historically. 🤔

Tryna think what the best option is here. It seems like a simple fix.

[matthew-dean]

I mean to go back to the original point:

Less shouldn't search for plugins in process.cwd(), but in same node_modules folder like less is installed.

How do you expect the Node environment to resolve that if you're running the binary from another non-relative folder? This is what I'm trying to understand. It's a non-trivial problem. Applications could be any layers of folders each with a node_modules folder, and Less itself, as I said, may not be installed as a direct child folder of whatever node_modules folder you're targeting. So, to make the assumption that you can run the binary from any other arbitrary folder and have it correctly resolve is quite an assumption. Whereas process.cwd() is indicating to the script that you want a node_modules/ folder relative to the "current" one that you're in.

🤔 hmm....

[matthew-dean]

I may have a way to fix this....

[matthew-dean]

Please look at #3200.