Skip to content

Conversation

puckowski
Copy link
Contributor

What:

Fixes variable interpolation issue after previous variable addition.

Why:

Current Less.js takes:

@a: 1px;
@b: 2px;
@c: @a + @b;

@radio-cls: radio;
@radio-cls-checked: @{radio-cls}_checked;

.@{radio-cls-checked} {
  border-color: #fff;
}

and produces:

._checked {
  border-color: #fff;
}

With fix the CSS is:

.radio_checked {
  border-color: #fff;
}

Checklist:

  • Documentation
  • Added/updated unit tests
  • Code complete

Tests passing locally. Updated tests for variables.

@dosubot dosubot bot added the size:S This PR changes 10-29 lines, ignoring generated files. label Nov 28, 2024
@puckowski
Copy link
Contributor Author

I submitted a few PRs. I expect rebase(s) will be needed in order to merge all of them, so let me know when a rebase is needed.

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Dec 7, 2024
* Fixes variable interpolation issue after previous variable addition.
* Fix rebase issue for fix for issue less#4258.
@puckowski
Copy link
Contributor Author

Rebased after merge conflict. Changes approved by one reviewer. Some CI looks flaky. Squashing and merging to avoid future merge conflicts.

@puckowski puckowski merged commit 304c310 into less:master Dec 8, 2024
6 of 8 checks passed
@matthew-dean
Copy link
Member

matthew-dean commented Feb 28, 2025

I didn't look at this issue carefully enough. @{radio-cls}_checked has not been valid Less syntax. Interpolation, historically, has only been allowed in quotes and identifiers. I think the reason it didn't throw an error is because of the fallback to a "permissive value" which then allows the interpolation but doesn't evaluate it. So, IMO, the fix for this bug is partially treating something that isn't a bug and adds another bug (unsupported feature) on top of it.

The clue that the fix is not valid is item.startsWith('@{') -- it doesn't logically follow that interpretation of a token would change based on its position IN the value, although I do know how we got here (because of the permissive fallback for variable values, which was probably not a good idea.

matthew-dean added a commit to matthew-dean/less.js that referenced this pull request Feb 28, 2025
matthew-dean added a commit that referenced this pull request Mar 1, 2025
* Added deprecation warnings

* Remove warning about combinator

* Switch to PNPM

* Update local dependencies

* Remove inner dist folder

* Create symbolic link to dist folder

* Flip dist folders

* Create symbolic link in root

* Add contributors

* 👥 Add @matthew-dean as a contributor

* 👥 Add @cloudhead as a contributor

* 👥 Add @lukeapage as a contributor

* 👥 Add @seven-phases-max as a contributor

* 👥 Add @iChenLei as a contributor

* 👥 Add @puckowski as a contributor

* Add more contributors

* Finish deprecation warnings

* Resolve deletions and such

* Fix symbolic link

* Update ci.yml to use PNPM

* Update ci.yml to use PNPM

* Refine CI versions

* Fix node printed version

* Better fix for #4258 and #4292

* Re-enable other tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm This PR has been approved by a maintainer size:S This PR changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants