Extension required in node_modules

[robhuzzey]

I've been trying to replicate an issue today & finally I have something by hacking around in my node_modules directory.

Basically, including a LESS file without an extension works, we know this... but... including without an extension from a node_modules directory fails.

I've created a simple repo demonstrating the following: https://github.com/robhuzzey/lessimporttest

@import './one/two/three/four/test'; works

as does: @import 'thisisinnodemodules/one/two/three/four/test.less';
(note that's including from node_modules with an extension)

However:

@import 'thisisinnodemodules/one/two/three/four/test';
(note that's including from node_modules without extension)

does not work and yields the following error:

$ npm run buildStyles

> lessissue@1.0.0 buildStyles /Users/robert.huzzey/Sites/test/lessissue
> lessc styles.less styles.css

FileError: 'thisisinnodemodules/one/two/three/four/test' wasn't found. Tried - /Users/robert.huzzey/Sites/test/lessissue/thisisinnodemodules/one/two/three/four/test.less,/Users/robert.huzzey/Sites/test/lessissue/thisisinnodemodules/one/two/three/four/test.less,/Users/robert.huzzey/Sites/test/lessissue/node_modules/thisisinnodemodules/one/two/three/four/test,thisisinnodemodules/one/two/three/four/test.less in /Users/robert.huzzey/Sites/test/lessissue/styles.less on line 5, column 1:
4 // This does NOT work
5 @import 'thisisinnodemodules/one/two/three/four/test';
6 

I've made a couple of branches to show this appears to be an issue with v3 but NOT v2... can anybody confirm if this is expected or undesired behaviour (I can't seem to find anything about this online).

[geuis]

Also ran into this issue today.

[acacha]

I confirm this issue also. You can reproduce it cloning and running npm run dev on this repo:

https://github.com/acacha/AdminLTETinkerLaravelMix

Take into account that this is maybe a webpack issue because using lessc command there are no problems.

[robhuzzey]

@acacha I did think the same thing but I have proved otherwise.. if you see the repo I created in the issue raised... you can see it's not using webpack & we do still have this issue :(

[robhuzzey]

I did have a dig around in the less source code & this looked likely a place to start testing some things out:

less.js/lib/less-node/file-manager.js

Lines 79 to 87 in 55380d4

	if (paths[i].indexOf('node_modules') > -1) {
	try {
	fullFilename = require.resolve(fullFilename);
	}
	catch (e) {}
	}
	else {
	fullFilename = options.ext ? self.tryAppendExtension(fullFilename, options.ext) : fullFilename;
	}

I'm reluctant to start digging or making a PR until I know this is isn't intended behaviour... if one of the maintainers of Less can confirm... I'm happy to investigate a fix.

[matthew-dean]

@robhuzzey Importing from node_modules was only added to the last 3.0 alpha, and no it's not intended behaviour that it wouldn't import without .less in the import. Definitely a bug in 3.0 alpha 3. Thanks for looking into it.

[robhuzzey]

@matthew-dean I seem to have fixed it with this work: https://github.com/robhuzzey/less.js/tree/issue3115_extInNodePath but am unclear on where to add a test for this?

All the tests pass with this change... I'm happy to submit the PR for review / suggestions?

[matthew-dean]

@robhuzzey You would need to add a .less NPM devDependency in package.json, and then add a test (in /tests/less) to load the file from a module (probably an import-module.less file?) After adding the test, I would verify that it breaks without the fix to make sure the test is working.

[robhuzzey]

Thanks @matthew-dean I did verify the current tests broke before putting in the fix... where I struggle is that all the tests were named .less.

I guess the next challenge is getting it to include a file from node_modules.

[matthew-dean]

@robhuzzey Would you also have time to look at #3116 ? That's fine if you don't.

[robhuzzey]

@matthew-dean sure... happy to help out.

I made a test for my branch here: https://github.com/robhuzzey/less.js/tree/issue3115_extInNodePath but something appears to not work as I expected (I think I don't fully understand how to include a test for this work).

If you can point me as to what I'm doing wrong, I would appreciate it :)

[robhuzzey]

Sorry @matthew-dean I'm working abroad at the moment so on a different timezone now... happy to look at the issues you have if you can let me know how to write tests for this lib? I've tried a few times periodically but feel like it's probably something obvious you can tell me with ease :)

[matthew-dean]

@robhuzzey I'm not familiar with the file: syntax in NPM, but, otherwise, it does appear to me like it should be running correctly. The only other thing you need to do is to place a import-module.css in test/css, which then is used as a comparison file with expected output so that the test can verify it's working. When you run grunt quicktest, you may need to adjust minor things in your output CSS like newlines or indents just to match Less output. You can even create an empty file for your first test and then copy / paste the expected output in the console log. As long as it's outputting the rulesets from modules with .less omitted, that's the important thing. I would actually blend your test so that some (or one) are imported with .less and some without, for a more robust test.

[robhuzzey]

@matthew-dean thank you... it was so simple in the end.. just needed the CSS to compare!

I didn't know about file:// syntax in NPM previously either... but now I realise this is a good use-case rather than making an actual NPM package just for a test :)

I've made the PR now: #3120 I'm not sure of best description... if you have examples I can follow etc or a template I will gladly follow it.

Thanks.

[robhuzzey]

This issue has been resolved by: #3120

[stefan-schweiger]

@robhuzzey Any timeline on when a new Alpha will be released with this fix? This currently breaks my webpack build when I try to override variables in an external library.

[matthew-dean]

@stefan-schweiger I'm going to try to release this either tonight or tomorrow night.

[matthew-dean]

If anyone is interested in getting involved in Releasing for the Less repo, by all means contact me here or on Twitter @matthewdeaners.

[robhuzzey]

@stefan-schweiger sorry, only just seen this. I've not got the ability to release like Matthew Dean has (this is my first contribution to Less to be honest).

@matthew-dean as you can probably tell, I'm quite busy at the moment but I have some scheduled "down time" coming up soon so I will come back to you then if that's ok? (I just cannot add more to what I'm currently doing right now).

[stefan-schweiger]

thanks @matthew-dean for fixing #3123, that's the reason I even got this error in my release build. So don't worry @robhuzzey it worked out by downgrading manually for before this was fixed.

[allentcm]

This problem still persisted in the release 3.0.1.
I manually downgraded to release 2.7.3 to resolve the issue.

[matthew-dean]

@allentcm That doesn't make sense. Less 2.7.3 doesn't import from the node_modules folder at all.

[babsonmatt]

@matthew-dean @allentcm downgrading to 2.7.3 worked for me as well

[Hellowor1d]

@matthew-dean @allentcm @babsonmatt downgrading to 2.7.3 worked for me , too 👍