Skip to content

fix(issue:4258) variable interpolation after math#4292

Merged
puckowski merged 2 commits intoless:masterfrom
puckowski:master_fix_4258
Dec 8, 2024
Merged

fix(issue:4258) variable interpolation after math#4292
puckowski merged 2 commits intoless:masterfrom
puckowski:master_fix_4258

Conversation

@puckowski
Copy link
Copy Markdown
Contributor

@puckowski puckowski commented Nov 28, 2024

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
Copy Markdown
Contributor Author

puckowski commented Nov 29, 2024

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
@puckowski
fix(issue:4258) variable interpolation after math
f10abf4 
* Fixes variable interpolation issue after previous variable addition.
@puckowski
chore: fix rebase issue
4b77818 
* Fix rebase issue for fix for issue less#4258.
@puckowski
Copy link
Copy Markdown
Contributor Author

puckowski commented Dec 8, 2024

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
@puckowski puckowski mentioned this pull request Jan 30, 2025
Closed
@matthew-dean
Copy link
Copy Markdown
Member

matthew-dean commented Feb 28, 2025 •
edited
Loading

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
Better fix for less#4258 and less#4292
13d1098
matthew-dean added a commit that referenced this pull request Mar 1, 2025
@matthew-dean
✨ feat: deprecation warnings for Less + monorepo chores (#4319)
d1abdab 
* 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

Reviewers

@matthew-dean matthew-dean matthew-dean approved these changes

Assignees

No one assigned

Labels

lgtm This PR has been approved by a maintainer size:S This PR changes 10-29 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@puckowski @matthew-dean