fix(ssr): fix adjacent text node concatenation #4974
Conversation
|
There is apparently some kind of concurrency issue... maybe we want to pull out the |
| @@ -34,6 +29,7 @@ export const expectedFailures = new Set([ | |||
| 'scoped-slots/mixed-with-light-dom-slots-inside/index.js', | |||
| 'scoped-slots/mixed-with-light-dom-slots-outside/index.js', | |||
| 'slot-forwarding/scoped-slots/index.js', | |||
| 'slot-not-at-top-level/advanced/ifTrue/light/index.js', | |||
There was a problem hiding this comment.
@wjhsf @jhefferman-sfdc I ended up having to do some big refactoring to avoid shared state in text node concatenation: f7a1e07
This led to a new test failure here because the cxt.nextSibling is actually wrong. This is something unrelated to this PR that we should fix independently. I can open a separate issue for that.
There was a problem hiding this comment.
Actually it looks like only adjacent text concatenation relies on cxt.nextSibling. 😆
There was a problem hiding this comment.
Wrote a minimal repro... let's deal with it later: 7161ea4
There was a problem hiding this comment.
Fixed that, wrote a better minimal repro... time to stop for real now 😛 34c5e04
| didBufferTextContent = true; | ||
| { |
There was a problem hiding this comment.
| didBufferTextContent = true; | |
| { | |
| { | |
| didBufferTextContent = true; |
In the context of this helper, seems weird to have that outside of the block statement. But in the context of the generated code, I guess it's more weird to have more code than necessary inside our wonky blocks. So I guess the way you wrote it is better.
Thanks for coming to my TED talk.
There was a problem hiding this comment.
I think we will probably end up refactoring this later anyway. If you look at the generated code, there is a ton of duplication, and Rollup does not inline the blocks as much as I'd like. We may end up just going back to the system of autogenerated variable names for performance reasons.
| textContentBuffer = ''; | ||
| didBufferTextContent = false; | ||
| } | ||
| `<EsBlockStatement>; |
There was a problem hiding this comment.
Not that it really matters, but would IfStatement or whatever be more accurate?
There was a problem hiding this comment.
You're right! 845edd6
Details
Fixes #4973
Does this pull request introduce a breaking change?
Does this pull request introduce an observable change?