fix(compiler): share logic for comments and whitespace #13550
Conversation
WalkthroughThis update introduces new utility functions for identifying whitespace and comment nodes in the template AST, refactors several core compiler modules and tests to use these utilities, and adds comprehensive tests for whitespace handling in text, slot, and transition transformations. No changes to public APIs or exported entities were made. Changes
Sequence Diagram(s)sequenceDiagram
participant Parser
participant Utils
participant Transformer
Parser->>Utils: isAllWhitespace(str)
Transformer->>Utils: isCommentOrWhitespace(node)
Transformer->>Utils: isWhitespaceText(node)
Note over Transformer,Utils: Utilities determine node type for whitespace/comment handling
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (9)
🧰 Additional context used🧬 Code Graph Analysis (5)packages/compiler-core/src/transforms/vIf.ts (1)
packages/compiler-core/src/transforms/vSlot.ts (1)
packages/compiler-core/__tests__/transforms/transformText.spec.ts (1)
packages/compiler-core/src/utils.ts (2)
packages/compiler-dom/src/transforms/Transition.ts (1)
⏰ Context from checks skipped due to timeout of 90000ms (5)
🔇 Additional comments (23)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Size ReportBundles
Usages
|
@vue/compiler-core
@vue/compiler-dom
@vue/compiler-sfc
@vue/compiler-ssr
@vue/reactivity
@vue/runtime-core
@vue/runtime-dom
@vue/server-renderer
@vue/shared
vue
@vue/compat
commit: |
|
/ecosystem-ci run |
edison1105
added
ready to merge
|
📝 Ran ecosystem CI: Open
|
This was intended as a refactoring, reusing logic for detecting comments and whitespace, but as it makes small modifications to how templates are compiled (especially for non-breaking spaces) I've marked it as a
fix.This PR only targets the compiler. I think similar changes should be made to the runtime, but those can be added later.
Background
While reviewing other PRs, I noticed a common pattern. There are a lot of places where we need a single child/root node, but we typically need to skip comments and whitespace text to find the node we want. Each place where this logic occurs implements it separately and I wanted to introduce some helper functions to reduce the duplication. Hopefully that will help to reduce inconsistencies, bugs and missed edge cases too.
There are a few common sources of problems:
whitespace: 'preserve'. This is closely related to handling comments as, in several cases, comments will also introduce whitespace text nodes that would otherwise be stripped.We also have an inconsistent definition of 'whitespace'.
Both
trim()and/\s/will treat many different characters as whitespace, including non-breaking spaces. For our purposes, this isn't really what we want, but we get away with it in practice.The template parser includes a function
isWhitespacethat only considers 5 characters to be whitespace: space, tab, newline, carriage return and form feed. From the perspective of collapsing and discarding whitespace nodes, this is the appropriate definition. It accounts for the characters that are typically used to format HTML code and that shouldn't be treated as meaningful content.A key part of this 'refactoring' is that
trim()is no longer used to identify whitespace text, withisWhitespacebeing preferred instead. The result is that some whitespace characters, most notably non-breaking space, will no longer be ignored in some contexts where they were being ignored previously. Non-breaking spaces will be treated just like any other normal text character.Examples
The examples below illustrate cases where something has changed in how this PR handles non-breaking spaces.
I don't think anyone will be intentionally using non-breaking spaces in the ways that are affected. If anyone is using them in these edge cases then it seems much more likely they've been included by accident and nobody noticed because the compiler ignored them. I believe these cases are rare, and it should be trivial to fix for anyone impacted.
The examples below use
for a non-breaking space, but it could also be included directly as a character, it doesn't need to be encoded as an entity.v-ifThis code would previously compile successfully, discarding the non-breaking space. With the changes in this PR it will throw an error, just as it would for non-whitespace text:
v-slotPreviously, the
would have been discarded. After this PR it will be treated as content of thedefaultslot.<Transition>Similarly, this will now fail to compile because the non-breaking space will be considered a child node:
Previously, the non-breaking space would have just been discarded.
Testing
The existing tests should all pass.
The tests I've added cover a variety of cases, many of which were already working previously. The tests that mention non-breaking spaces are typically testing an actual change.
Some parts of this are a bit tricky to test effectively, as they require specific combinations of compiler options and transforms. In particular, testing cases involving
TEXT_CALLnodes, which are only created bytransformText.Related PRs
There are various open PRs that attempt to fix problems related to the handling of comments and whitespace nodes. In particular, these two may benefit from using the utility functions added in this PR:
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Refactor