Plugin loading fails when using syncImport

[yannickl88]

Loading a plugin fails when less is using the syncImport option. The File manager seems to return the result instead of a promise in this case. It can be easily reproduced with the following code:

Example:

// pi.js
module.exports = {
    install: function(less, pluginManager, functions) {
        functions.add('pi', function() {
            return Math.PI;
        });
    }
};

// index.js
var less = require('less');

less.render('@plugin "./pi.js"; @var pi()', { syncImport: true }, function (error, output) {
    console.log(error, output);
});

Error:

TypeError: fileManager.loadFile(...).then is not a function
    at /home/ydelange/less-bug/node_modules/less/lib/less-node/plugin-loader.js:34:72
    at new Promise (<anonymous>)
    at PluginLoader.loadPlugin (/home/ydelange/less-bug/node_modules/less/lib/less-node/plugin-loader.js:33:12)
    at ImportManager.push (/home/ydelange/less-bug/node_modules/less/lib/less/import-manager.js:149:36)
    at ImportVisitor.processImportNode (/home/ydelange/less-bug/node_modules/less/lib/less/visitors/import-visitor.js:88:28)
    at ImportVisitor.visitImport (/home/ydelange/less-bug/node_modules/less/lib/less/visitors/import-visitor.js:50:22)
    at Visitor.visit (/home/ydelange/less-bug/node_modules/less/lib/less/visitors/visitor.js:76:32)
    at Visitor.visitArray (/home/ydelange/less-bug/node_modules/less/lib/less/visitors/visitor.js:102:22)
    at Ruleset.accept (/home/ydelange/less-bug/node_modules/less/lib/less/tree/ruleset.js:40:30)
    at Visitor.visit (/home/ydelange/less-bug/node_modules/less/lib/less/visitors/visitor.js:83:18)

Code that is causing it:
https://github.com/less/less.js/blob/master/lib/less-node/file-manager.js#L42

[matthew-dean]

This certainly sounds like a bug, but just curious, why are you using syncImport for plugins?

[yannickl88]

@matthew-dean I'm not specificly using syncImport for plugins, just for the the render method in general. I'm just using the API as specified in the docs: less.render('...', { syncImport: true }, function(error, output) {});.

EDIT: on second though, you might be more interested in the conext. I'm building a build tool for our web applications. The tool, for now, only has a simple callback per build step. So each step needs to be finished when the callback ends. So loading stuff async isn't really an option.

[yannickl88]

BTW, I can provide a fix to solve this issue, I just could not make sense of the tests to provide a regression test.

If you are willing to help me with the test, I'm happy to provide a PR for this issue.

[matthew-dean]

@yannickl88 Yep, I could probably help with tests. In general, the test/index.js file runs all tests, and these 2 lines here in particular run with syncImport - https://github.com/less/less.js/blob/master/test/index.js#L77

So, if in your PR, you added a @plugin reference to one of the files in the test/less/import, that should cover regression.

If it changes output, you'll need to update the test/css files to match output. Does that help?

[yannickl88]

Thanks! I will have a look tomorrow when I'm back in the office. I don't have my change here on this PC. If I need any help, I'll let you know.

[yannickl88]

hmm, I've added a regression test. However, my 'fix' does not appear to be sync enough. Which looks like it might be doing something async further on.

My Changes: master...yannickl88:fix/3294

I suspect it has something to do with https://github.com/less/less.js/blob/master/lib/less/import-manager.js#L41 which does not return anything.