fix(ssr-compiler): transform of empty text strings #4963
Conversation
|
|
||
| cxt.import('htmlEscape'); | ||
| return bYieldEscapedString(tempVariable, valueToYield, isIsolatedTextNode); | ||
| return [bYieldEscapedString(valueToYield, isIsolatedTextNode)]; |
There was a problem hiding this comment.
| return [bYieldEscapedString(valueToYield, isIsolatedTextNode)]; | |
| return [bYieldEscapedString(valueToYield)]; |
Looks like tests are failing because we don't need the extra variable any more.
There was a problem hiding this comment.
Also we can change the return type of this function to EsBlockStatement[] to be a bit more precise.
There was a problem hiding this comment.
The variable is declared as export const Text: Transformer<IrText>, so the return type on the function doesn't actually matter. The return type defined by Transformer<IrText> is what will ultimately be used.
There was a problem hiding this comment.
Looking good so far!
For future reference, we are adding new failing tests because the new code is smaller, more concise, and more similar to the pre-existing api.ts code, even if we're introducing new test failures which are still due to adjacent text node collapsing not being implemented.
| 'dynamic-imports/basic/index.js', | ||
| 'context-no-provider/index.js', | ||
| 'attribute-global-html/as-component-prop/with-@api/index.js', | ||
| 'adjacent-text-nodes/nonempty/index.js', |
There was a problem hiding this comment.
Let's alphabetize these
| yield ${0} ? htmlEscape(${0}.toString()) : ${2} ? '\\u200D' : ''; | ||
| { | ||
| const value = ${/* string value */ is.expression}; | ||
| const massagedValue = value == null ? '' : String(value); |
There was a problem hiding this comment.
Let's add a comment with a link to the code in api.ts to make it clear why we're doing this particular logic:
There was a problem hiding this comment.
@nolanlawson for the suggestion about function return type: EsBlockStatement might be too specific as it can also return an IRLiteral[]
There was a problem hiding this comment.
Ah, missed that. OK no problem.
|
|
||
| cxt.import('htmlEscape'); | ||
| return bYieldEscapedString(tempVariable, valueToYield, isIsolatedTextNode); | ||
| return [bYieldEscapedString(valueToYield, isIsolatedTextNode)]; |
There was a problem hiding this comment.
Also we can change the return type of this function to EsBlockStatement[] to be a bit more precise.
…ected-failure additions
|
Fixed merge conflicts with #4965. We still have 3 new failures but that's fine; the new code is more succinct and we have more work to do to fix adjacent text nodes anyway. |
|
Actually I'm wrong – I just had a bug in my merge commit. No new test failures, and smaller code. Hooray! |
Details
Aligns transformation of empty text strings with original implementation
Does this pull request introduce a breaking change?
Does this pull request introduce an observable change?
GUS work item
W-17094045