Fix import failure when using the syncImport option

[maxbrunsfeld]

⚠️ I've determined that this fix isn't correct. See my comment below ⚠️

I'm hitting a strange bug when using the syncImport option to less.render.

I wasn't sure how to contribute an automated test for this bug, so I have come up with a fairly minimal way to reproduce it. It involves three less files in a directory:

mixins.less:

.some-mixin() {
  @import "variables";
}

variables.less:

@some-color: red;

test.less

@import "mixins";
@import "variables";

.some-selector { color: @some-color; }

The following test script demonstrates the problem. It can be run from the same directory as the 3 less files above.

const fs = require('fs'),
      less = require('less'),
      filename = 'test.less',
      data = fs.readFileSync(filename, 'utf8')

less.render(data, {filename: filename, syncImport: false}, function (err, result) {
  console.log('async imports. error:', err)
  less.render(data, {filename: filename, syncImport: true}, function (err, result) {
    console.log('sync imports. error:', err)
  })
})

Expected output:

async imports. error: null
sync imports. error: null

Actual output:

async imports. error: null
sync imports. error: { [Error: variable @some-color is undefined]
  type: 'Name',
  filename: 'test.less',
  index: 64,
  line: 4,
  callLine: NaN,
  callExtract: undefined,
  column: 24,
  extract: [ '', '.some-selector { color: @some-color; }', '' ],
  message: 'variable @some-color is undefined',
  stack: undefined }

Let me know if I can provide anything else to help get this fixed. Thanks!

[maxbrunsfeld]

Also, if it's helpful, I don't think this bug existed in v2.0.0.

[maxbrunsfeld]

@matthew-dean @lukeapage After looking into this a bit more, I think the problem is the way the ImportManager dedupes imports. It looks like after it visits the import of variables.less inside of the mixin-definition in mixins.less, it considers the second import of variables.less inside of test.less to be a duplicate, even though it occurs in a different context (the top-level context).

Should context be taken into account when deduping imports?

[matthew-dean]

If you want Less to import into both contexts, you can use @import (multiple). Otherwise this is expected behavior.

[maxbrunsfeld]

Otherwise this is expected behavior.

It's the expected behavior that the files compile with syncImport: false and fail to compile with syncImport: true? Are you sure?

Note that the first import occurs only inside the body of the mixin, whereas the second one occurs in the top-level context. I would expect this to cause the variable to be defined in the top-level context. This is what happens with syncImport: false, but not with syncImport: true.

[matthew-dean]

@maxbrunsfeld Oh, no you're right. That shouldn't happen. I mis-read your comment. It should not compile in either scenario.

[maxbrunsfeld]

@matthew-dean Ok, thanks for clarifying. So in general, @import is a no-op if the file has already been imported before, even if it was previously imported into a different nested context.

[matthew-dean]

@maxbrunsfeld That's the current behavior, yes, AFAIK. However.... I wonder if subsequent imports should not be treated as something like @import (multiple, reference). That is, output nothing, but allow definitions to be exposed. (Since the reason this behavior is turned off by default is to prevent outputting styles multiple times.)