-
-
Notifications
You must be signed in to change notification settings - Fork 8.8k
refactor(compiler-vapor): skip unnecessary attribute quoting in templates #13673
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
base: minor
Are you sure you want to change the base?
Conversation
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing Touches🧪 Generate unit tests
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 (
|
❌ Deploy Preview for vue-sfc-playground failed. Why did it fail? →
|
needsQuoting = /[\s>]|^["'=]/.test(value) | ||
|
||
if (needsQuoting) { | ||
const encoded = value.replace(/"/g, '"') |
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 a partial fix for value='"foo"'
being turned into value=""foo""
, I don't think this is the way to do it though, especially since this doesn't escape &
as well.
I've looked into how Vue compiler deals with entities and it seems that we only ever do decoding (which makes sense for the original compiler, but it seems bad for Vapor where we generate HTML templates, we don't want static HTML code examples to be parsed as anything but text)
// https://html.spec.whatwg.org/multipage/introduction.html#intro-early-example | ||
needsQuoting = /[\s>]|^["'=]/.test(value) |
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've noticed that the browser diverges slightly from the specification here, the backticks and open angled bracket aren't something browser parsers cares about, and single/double quotes only matters if it's at the start of the value.
This can be tested on the console:
template = document.createElement('template');
template.innerHTML = '<div data-backticks=`good data-end-quote=good" data-open-bracket=<good></div>';
template.innerHTML;
Let me know if we should just align to what the specification says, or to add a note that we're diverging and matching the browser.
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.
When you say "the browser" have you tested that that works in ALL browsers? If not then it would be better to follow the spec to avoid breaking things.
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.
yes, works fine in Gecko, WebKit and Blink. I can't test in Ladybird and Servo at the moment but I'd be surprised if it doesn't. this seems like one of those implementation-level quirk that ends up getting reimplemented across browsers, either that or this quirk is actually documented elsewhere and not on the page I referenced
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 behavior is not directly tested by WPT, but they pull some tests from html5lib which has (among the tests not included in WPT at this time) tests for these quirks, which seem to indicate that it's invalid but tokenized properly (and therefore likely to gracefully be accepted by browsers as-is). https://github.com/html5lib/html5lib-tests/blob/a9f44960a9fedf265093d22b2aa3c7ca123727b9/tokenizer/test3.test#L10461-L10466
While it's not as convincing as it could be given the fact it's not pulled by WPT, Servo does uses them
Thanks for your PR, it looks great. I think it should add some runtime tests in |
b443817
to
ba9cd1c
Compare
Let me know if these should just be two separate test cases |
This PR alters the template generation in Vapor compiler to omit unnecessary quoting of attribute values and inclusion of whitespace between attributes, which when combined with #13667 can save a decent amount of bytes
<div id="foo" class="bar"></div>
<div id=foo class=bar></div>
<div id="foo>bar" class="has whitespace"></div>
<div id="foo>bar"class="has whitespace"></div>