fix(compiler): share logic for comments and whitespace #13550
Conversation
WalkthroughAdds utilities to detect whitespace/comment nodes, updates parser and several transforms (v-if, v-slot, transition) to use them, and extends tests to cover whitespace and comment handling in text, slots, v-if adjacency, and transition children. Changes
Sequence Diagram(s)sequenceDiagram
participant Parser
participant Utils
participant Transforms
participant Tests
Parser->>Utils: call isAllWhitespace(text)
note right of Utils: provides centralized whitespace test
Transforms->>Utils: call isCommentOrWhitespace(node)
Transforms->>Utils: call isWhitespaceText(node)
note right of Transforms: vIf / vSlot / Transition use utilities to\nfilter/identify whitespace and comments
Tests->>Parser: parse templates with whitespace/comments
Tests->>Transforms: run transforms (with transformText enabled in some tests)
note over Tests,Transforms: assertions updated/added to validate behavior
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Potential hotspots to inspect:
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
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: |
edison1105
added
ready to merge
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/compiler-core/__tests__/transforms/vSlot.spec.ts (1)
31-51: transformText toggle in parseWithSlots is sound; optional cleanup of options spreadConditionally appending
transformTexttonodeTransformsvia an extratransformText?: booleanflag lets tests opt into the full pipeline without affecting existing cases. If you want to keep this flag purely test-local, you could optionally destructure it out before passing...optionsintoparse/transformso it never reaches compiler APIs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/compiler-core/__tests__/transforms/vIf.spec.ts(3 hunks)packages/compiler-core/__tests__/transforms/vSlot.spec.ts(5 hunks)packages/compiler-core/src/parser.ts(1 hunks)packages/compiler-core/src/transforms/vIf.ts(2 hunks)packages/compiler-core/src/transforms/vSlot.ts(3 hunks)packages/compiler-core/src/utils.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/compiler-core/src/parser.ts
- packages/compiler-core/src/transforms/vIf.ts
- packages/compiler-core/src/transforms/vSlot.ts
- packages/compiler-core/src/utils.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/compiler-core/__tests__/transforms/vSlot.spec.ts (2)
packages/compiler-core/src/transforms/transformText.ts (1)
transformText(18-119)packages/compiler-core/src/ast.ts (2)
ObjectExpression(374-377)SimpleExpressionNode(225-247)
🔇 Additional comments (5)
packages/compiler-core/__tests__/transforms/vIf.spec.ts (2)
269-293: Expanded v-else adjacency coverage (including NBSP) is accurateThe added cases for
<div v-if="bar"/>foo<div v-else/>and the NBSP variant correctly assertX_V_ELSE_NO_ADJACENT_IFand use the rightmock.calls[3]/mock.calls[4]indices, aligning tests with the new definition that non‑breaking spaces are not ignorable whitespace.
333-371: v-else-if non-adjacent and NBSP tests align with intended semanticsThe mirrored v-else-if cases (with text and NBSP between branches) and the updated expectation at
mock.calls[5]correctly exercise the non-adjacent branch error path and ensure the “else-if after else” case still targets the final branch’sloc.packages/compiler-core/__tests__/transforms/vSlot.spec.ts (3)
315-347: NBSP before a named slot correctly forms an implicit default slotThe new “named slots w/ implicit default slot containing non-breaking space” test accurately asserts that a lone NBSP outside the named
<template #one>becomes default slot content (TEXTwithcontent: ' \u00a0 '), while the named slot still returns onlyfoo. This matches the intent that NBSP is treated as real text rather than ignorable whitespace.
1053-1072: Preserve-whitespace + NBSP default slot behavior is well coveredWith
whitespace: 'preserve', asserting that a template containing plus a named#headertemplate produces adefaultslot entry (and matching snapshot) cleanly captures the new behavior that NBSP-only content should not be discarded as whitespace-only.
1088-1104: transformText + comments between named v-if / v-else slots are now exercisedThe “named slot with v-if + v-else and comments” test, using
transformText: trueandwhitespace: 'preserve', is a good addition to ensure the combination of comments, preserved whitespace, and transformText doesn’t break v-if/v-else branching for slots; the snapshot-based assertion is appropriate here.
|
/ecosystem-ci run |
|
📝 Ran ecosystem CI: Open
|
3 participants
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
✏️ Tip: You can customize this high-level summary in your review settings.