Fixes less#3675 - LESS detects @apply as variable (ISSUE)

[opike]

Issue #3675 - LESS detects @apply as variable (ISSUE)

Issue was identified as a bug

Checklist:

• [ ] Documentation N/A
• [x] Added/updated unit tests
• [x] Code complete

This is my first PR - all the tests are passing and I added a new test that is confirmed to fail without the changes.

[opike]

My bad with the failing tests. Something was off with my set up prior to submitting the PR where for some reason only a subset of the tests were being run and they were all passing:

I'm going back and taking another look now.

[opike]

I updated the fix however there's still one test that is failing, which I don't yet have a clear idea how to resolve. [LessError: '@file_to_import' wasn't found. Tried - /Volumes/github-image/less.js/packages/test-data/less/_main/@file_to_import.less,/Volumes/github-image/less.js/packages/test-data/less/_main/@file_to_import.less,npm://@file_to_import,npm://@file_to_import.less,@file_to_import.less] { stack: undefined, type: 'File', filename: '/Volumes/github-image/less.js/packages/test-data/less/_main/urls.less', index: 2168, line: 70, column: 0, callLine: NaN, callExtract: undefined, extract: [ '.add_an_import(@file_to_import) {', '@import "@{file_to_import}";', '}' ] }

With this code: .add_an_import(@file_to_import) { @import "@{file_to_import}"; }

A distinction needs to be made between the @{file_to_import} variable reference and an arbitrary at-rule such as @apply. Presuming the overall direction I took is appropriate with the fix, I believe that could be done through the context in this code block:

https://github.com/opike/less.js/blob/7cf515abac3fdac9f3be8adfad62734b01ccc60a/packages/less/src/less/tree/quoted.js#L36

Although it's not yet apparent to me specifically where in the context to key off of.

[matthew-dean]

So, if I recall correctly, what I originally did was basically hack custom properties to consider the whole value as a kind of "escaped quoted value" so that it wouldn't try to tokenize any particular bit, and then would over-rode the regex statements to treat "variable-like" statements as variable lookups.

Now, I'm not sure the latter part was the best idea (maybe we should have required at-rule tokens to be escaped, just like in a quote, if in a custom property), but one thing with your change -- will it not miss errors now with variables that are in quotes?

[matthew-dean]

In regards to this:

.add_an_import(@file_to_import) {
  @import "@{file_to_import}";
}

...where are you getting this code block from?

[opike]

That code block is from here: https://github.com/less/less.js/blob/4d3189c05175dfd8aab505ec19c7f5724f145295/packages/test-data/less/_main/urls.less#L69

With my change with the following code: .test { --iron-autogrow-textarea: { @apply "${some_variable}"; } }

The error "NameError: Property '$some_variable' is undefined in " is thrown. Is that the scenario you were describing?

On Fri, May 12, 2023 at 12:11 PM Matthew Dean @.***> wrote:

In regards to this:

.@.***_to_import) { @import "@{file_to_import}"; }

...where are you getting this code block from?

— Reply to this email directly, view it on GitHub https://github.com/less/less.js/pull/3798#issuecomment-1546167589, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAVRLC6HCRNIWEBALWGPEHLXF2DODANCNFSM6AAAAAAX6WPVJY . You are receiving this because you authored the thread.Message ID: @.***>

[opike]

For the block with file_to_import, it looks like there's some async behavior going on since the execution path is different with and without the debugger.

On Fri, May 12, 2023 at 12:57 PM Oliver Pike @.***> wrote:

Sorry, this was the code block I used: .test { --iron-autogrow-textarea: { @apply "${some_variable}"; } }

On Fri, May 12, 2023 at 12:56 PM Oliver Pike @.***> wrote:

That code block is from here:

https://github.com/less/less.js/blob/4d3189c05175dfd8aab505ec19c7f5724f145295/packages/test-data/less/_main/urls.less#L69

With my change with the following code: .test { --iron-autogrow-textarea: { @apply "${file_to_import}"; } }

The error "NameError: Property '$some_variable' is undefined in " is thrown. Is that the scenario you were describing?

On Fri, May 12, 2023 at 12:11 PM Matthew Dean @.***> wrote:

In regards to this:

.@.***_to_import) { @import "@{file_to_import}"; }

...where are you getting this code block from?

— Reply to this email directly, view it on GitHub https://github.com/less/less.js/pull/3798#issuecomment-1546167589, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAVRLC6HCRNIWEBALWGPEHLXF2DODANCNFSM6AAAAAAX6WPVJY . You are receiving this because you authored the thread.Message ID: @.***>

[matthew-dean]

@opike Oh, I see. I wasn't aware variable replacement worked in an import in a mixin call. Imports are very tricky (and using variable replacement in them leads to unexpected results) because the import blends variable scope. So an import may rely on a variable defined in another import, and whether or not to import that import may rely on yet another variable.

So, imports are sort of evaluated early in the parsing cycle and, to be honest, I'm not sure exactly how they work.