Closed pgoldberg closed 1 year ago
Hey @matthew-dean and @iChenLei! If one of you gets a chance to review this sometime soon I'd really appreciate it! This will fix a pretty bothersome bug we're seeing (and it looks like others have run into it, too: https://github.com/less/less.js/issues/3567)
Hey @matthew-dean and @lumburr, I just wanted to ping on this PR again since I think it is a somewhat major bug in less and we'd really like to be able to pick up this fix. Thank you!
@matthew-dean @lumburr I totally understand you are probably pretty busy and that less is only lightly maintained at this point, but I would really appreciate one of you taking a look at this bugfix. Is there anything I could do to help speed up the process here/make things easier for you to review? Thank you both for maintaining!
@pgoldberg Only Matthew-Dean has the authority to release a new version for Less currently. I can review and merge, but no npm permission.
However, you can use the patch-package tool to apply your changes to Less before Matthew-Dean reviews and releases a new version.
@iChenLei @pgoldberg Sorry y'all. Yeah, I work full time and am a single parent, so Less doesn't get a lot of love. I would love to hand over Less to a new group and can help steward that.
I don't know the implications of adding other NPM publishing rights, but I can do that.
As for this, @iChenLei do you have approval permissions for PRs? It would be nice to know that code like this has some review and sign-off.
In the meantime, my intuition @pgoldberg is that your research is correct, because Less doesn't have robust coverage around source map validation, so I'll just trust that this is more correct than what existed.
In addition, even though I'm responsible for the parseNode
section that does re-parsing of some nodes post evaluation, I now feel that's mostly unnecessary and a mistake. I was able to create a minimal Less-like language that evaluates variables within selectors, and successfully re-combines it into selectors without re-parsing. (Re-parsing also doesn't cleanly separate parsing from evaluation, which makes improving / replacing the parser trickier in the future.)
In the meantime, I will approve / merge this code.
Btw, @pgoldberg if you have any interest in being added to the collaborator list for Less, I'd be happy to add you.
@matthew-dean Thank you!! Would it be possible to get a new release with this change?
Sorry y'all. Yeah, I work full time and am a single parent, so Less doesn't get a lot of love. I would love to hand over Less to a new group and can help steward that.
No worries – totally understand. Thank you for taking a look and merging!!
Btw, @pgoldberg if you have any interest in being added to the collaborator list for Less, I'd be happy to add you.
Thank you for the offer! I think I will probably pass just since I don't feel extremely familiar with much of the less code (although this was definitely a good introduction!) 🙂
Note to reviewer: There are two potential solutions here, one that fixes the specific issue linked, and one that might be a bit more robust. Filter down to the first commit for the bandaid fix, second commit for the more robust one, third commit for added test. I recommend hiding whitespace changes in the GitHub UI – my editor removed a bunch of unnecessary trailing spaces
PR Template
What:
Fixes https://github.com/less/less.js/issues/3567
Source maps are pointing to the wrong files, and it seems that it points at different files on different runs, causing non-deterministic source map output.
Checklist:
Why and How are included in the below description, which details the problem in the code and the proposed solutions.
Investigation and the problems
Bear with me a bit, this was my first venture into the less source code 🙂
We were seeing that we got weird source map output, mapping to files that shouldn't even appear in the source maps (e.g. reference imports from which nothing was used). After digging, there are a couple of problems that are causing this whole issue.
Background information
I was able to narrow down my search a bit by finding when this bug was introduced. It was released in
3.5.0-beta.2
, and the bug was introduced in https://github.com/less/less.js/pull/3227.We were seeing this happen when a selector included a variable, which directed me to this bit: https://github.com/less/less.js/pull/3227/files#diff-d8c0204835f49ae90096efe1e2d0d80868e0e6214bfd4c960a097eb20cc14ec9R67-R83
It looks like what this is doing is recreating the selector CSS with the "variableCurly" node replaced with the variable's value. Then it parses that newly created CSS, and replaces the old
Selector
node(s) with the new one(s). It looks like the code is trying to preserve source information by passing in thefileInfo
andindex
of the original selectors toparseNode
: https://github.com/less/less.js/blob/eefe33a47f6fdcc228817df7435a1770ce9e51ea/packages/less/src/less/tree/ruleset.js#L85-L86For a little more context, my understanding is that
parseNode
is used for creating new nodes after the original input files are parsed.First problem
In
parseNode
, the providedcurrentIndex
andfileInfo
are added to the newly created node(s) here: https://github.com/less/less.js/blob/eefe33a47f6fdcc228817df7435a1770ce9e51ea/packages/less/src/less/parser/parser.js#L111-L112These lines assume that
result
is a tree node. However, in some cases,result
is actually an array of nodes. So when it tries to add the source info from the old selectors to the new ones, it's actually just setting_index
and_fileInfo
properties on the array itself, so it doesn't actually ever make it to the source maps. In this case, theparseList
is["selectors"]
, soresult
is an array ofSelector
tree nodes.Second problem
Selector
nodes have anelements
array, containing the elements that make up the selector. When aSelector
is added to the output, it actually does so by generating the CSS for each of its elements: https://github.com/less/less.js/blob/eefe33a47f6fdcc228817df7435a1770ce9e51ea/packages/less/src/less/tree/selector.js#L135-L138This means that if the
_fileInfo
or_index
for the Selector's elements is incorrect, then the output source map will be incorrect. That also means that even if the_fileInfo
and_index
were correctly set on theSelector
(s) rather than the array containing theSelector
(s), the source maps would still be incorrect on theElement
s that make up the selector. The source info for the elements gets set here: https://github.com/less/less.js/blob/eefe33a47f6fdcc228817df7435a1770ce9e51ea/packages/less/src/less/parser/parser.js#L1299There are two problems here:
currentIndex
passed intoparseNode
doesn't get added to the createdElement
s indexfileInfo
is not thefileInfo
that gets passed intoparseNode
. It's thefileInfo
that's given to theParser
when it's instantiated: https://github.com/less/less.js/blob/eefe33a47f6fdcc228817df7435a1770ce9e51ea/packages/less/src/less/parser/parser.js#L41The proposed solution(s)
I have two proposals for how we can solve this. One is more of a bandaid fix, and the other is (I believe) a bit more robust. Both solutions include the fix for where
_fileInfo
and_index
get set on an array.elements
for each selector returned byparseNode
, and update the_fileInfo
and add thecurrentIndex
to the_index
for each one.this.parse.parseNode
and relying onparseNode
to update the_index
and_fileInfo
of the created nodes, create a newParser
object each time we want to callparseNode
. This removes thecurrentIndex
andfileInfo
params forparseNode
, and adds acurrentIndex
param to theParser
itself. All nodes created by the parser will addcurrentIndex
(which defaults to 0) to the index fromparserInput
. This way, we can ensure that any nodes created withparseNode
will have the correct source information.The first proposed solution can be seen by filtering to the first commit, and the second proposed solution can be seen by filtering to both the first and second commits. The third commit adds a test to ensure that selectors with variables in them have properly generated source maps. I recommend hiding whitespace changes in the GitHub UI – my editor removed a bunch of unnecessary trailing spaces.