-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
amp-bind: Support special chars in strings #23811
amp-bind: Support special chars in strings #23811
Conversation
ba3f568
to
97210de
Compare
/to @jridgewell |
@@ -205,6 +205,17 @@ describe('BindExpression', () => { | |||
expect(evaluate('+"1"')).to.equal(1); | |||
}); | |||
|
|||
it('should parse special characters', () => { | |||
expect(evaluate('"\\n"')).to.equal('\n'); |
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.
What does this look like when written in HTML?
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.
Looks like "\n"
.
const string = yytext.substr(1, yyleng - 2); | ||
// Replace leading/trailing single-quote with double-quote chars and | ||
// use JSON.parse() to process special chars e.g. '\n'. | ||
const string = parseJson(`"${yytext.substr(1, yyleng - 2)}"`); |
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.
Is this safe? What if we used a single quote and the string includes double quotes inside it?
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.
Attribute values can't have double quotes.
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.
Note, this will also transform unicode escapes (\u0000
) into their unicode chars.
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.
Yea, that's #22265.
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.
<div attr='foo"bar'>
is legal HTML
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.
Excellent catch. 👍
const string = yytext.substr(1, yyleng - 2); | ||
// Replace leading/trailing single-quote with double-quote chars and | ||
// use JSON.parse() to process special chars e.g. '\n'. | ||
const string = parseJson(`"${yytext.substr(1, yyleng - 2)}"`); |
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.
Note, this will also transform unicode escapes (\u0000
) into their unicode chars.
const string = yytext.substr(1, yyleng - 2); | ||
// Replace leading/trailing single-quote with double-quote chars and | ||
// use JSON.parse() to process special chars e.g. '\n'. | ||
const string = parseJson(`"${yytext.substr(1, yyleng - 2)}"`); |
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.
<div attr='foo"bar'>
is legal HTML
* Support special chars. * Fix single quote strings. * Fix presubmit, add test. * Add test for escaping special chars. * Escape double-quotes in strings and fallback to non-parsed string. * Use regex with /g. * One more test case.
Fixes #17863 and fixes #22265.