-
Notifications
You must be signed in to change notification settings - Fork 403
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(template-compiler): avoid discarding text content whitespace for complex expressions #4029
Conversation
the HTML whitespace while preserving text content whitespace, while also taking into account how | ||
comments are sometimes preserved (in which case we need to keep the HTML whitespace). | ||
*/ | ||
if (!rawText.trim().length && !ctx.config.experimentalComplexExpressions) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there existing test cases that cover the usage for this change?
Just to confirm, with experimentalComplexExpressions
enabled, the text processing goes through a separate processing path and rawText
contains the whitespace that should be kept right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think we should take this entire test:
Lines 3 to 11 in 57272ef
<div>{noSpace}</div> | |
<div>{spaceRight} </div> | |
<div> {spaceLeft}</div> | |
<div>{one}{two}</div> | |
<div> {one}{two}</div> | |
<div>{one}{two} </div> | |
<div>{one} {two}</div> | |
<div> {one} {two}</div> | |
<div>{one} {two} </div> |
...and run it through Karma to see what the actual rendered textContent
is.
Ideally, we should not change how whitespace has been handled historically in LWC. I think it would be too disruptive, even if we put complex expressions behind API versioning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ran the same test with the configuration disabled and got the same expected output, although the AST changes. The AST with the changes in this PR looks incorrect, let me look into it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like it's because we're doing sub-parsing using acorn. This sounds like a difficult problem...do we care about these AST snapshots being wonky if we're getting the expected rendered output?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an example of the AST discrepancy I mentioned earlier:
"property": {
"type": "Identifier",
"start": 166,
"end": 171,
"loc": {
"start": {
"line": 6,
"column": 22
},
"end": {
"line": 6,
"column": 27
}
},
"range": [
166,
171
],
"name": "title"
},
"computed": false,
"optional": false,
"location": {
"startLine": 6,
"startColumn": 17,
"endLine": 6,
"endColumn": 29,
"start": 160,
"end": 172
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nolanlawson I confirmed that the whitespace renders the same in the browser with the feature enabled/disabled.
Details
The logic used to discard unnecessary HTML whitespace was being applied to text content whitespace due to a change in the timing that the text content whitespace is being parsed. Detailed comment added for posterity when we need to integrate the complex expression changes.
Does this pull request introduce a breaking change?
Does this pull request introduce an observable change?
GUS work item
W-14631165