-
Notifications
You must be signed in to change notification settings - Fork 24.8k
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
fix(compiler): don't report parse error for interpolation inside string in property binding #40267
fix(compiler): don't report parse error for interpolation inside string in property binding #40267
Conversation
…ng in property binding Currently we check whether a property binding contains an interpolation using a regex so that we can throw an error. The problem is that the regex doesn't account for quotes which means that something like `[prop]="'{{ foo }}'"` will be considered an error, even though it's not actually an interpolation. These changes build on top of the logic from angular#39826 to account for interpolation characters inside quotes. Fixes angular#39601.
this._forEachUnquotedChar(input, 0, charIndex => { | ||
if (startIndex === -1) { | ||
startIndex = input.indexOf(start, charIndex); | ||
return false; | ||
} else { | ||
endIndex = this._getInterpolationEndIndex(input, end, charIndex); | ||
return endIndex > -1; | ||
} | ||
}); |
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 think that this is a bit inefficient, since it will continue to run through each unquoted character even indexOf
found the start.
Also I am concerned that the following string would find a false positive:
abc '{{' }} def
since the {{
is in a string, it should be ignored but would be picked up by the indexOf()
, when the charIndex
is 0
.
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 think this would also be clearer if you created a new function _getInterpolationStartIndex(input, expressionStart, start)
:
private _getInterpolationStartIndex(input: string, expressionStart: string, start: number): number {
let result = -1;
this._forEachUnquotedChar(input, start, charIndex => {
if (input.startsWith(expressionStart, charIndex)) {
result = charIndex;
return true;
} else {
return false;
}
});
return result;
Then you could do:
const startIndex = this._getInterpolationStartIndex(input, start, 0);
if (startIndex === -1) {
return;
}
const endIndex = this._getInterpolationEndIndex(input, end, startIndex+start.length);
if (endIndex === -1) {
return;
}
this._reportError(...);
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 didn't want to add a _getInterpolationStartIndex
, because there's some other logic that looks for an interpolation start in splitInterpolation
, but the requirements there are slightly different. As for the abc '{{' }} def
case, I think I tried something similar and it was reported as a different (syntax?) error further down the compilation process.
…de string in property binding
I've added the extra test cases and addressed the feedback. @petebacondarwin |
…de string in property binding
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.
Thanks for the changes @crisbeto. One last thing.
…de string in property binding
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.
Great! Thanks for the quick turnaround.
FYI, presubmit is successful for the changes in this PR. |
…ng in property binding (#40267) Currently we check whether a property binding contains an interpolation using a regex so that we can throw an error. The problem is that the regex doesn't account for quotes which means that something like `[prop]="'{{ foo }}'"` will be considered an error, even though it's not actually an interpolation. These changes build on top of the logic from #39826 to account for interpolation characters inside quotes. Fixes #39601. PR Close #40267
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Currently we check whether a property binding contains an interpolation using a regex so that we can throw an error. The problem is that the regex doesn't account for quotes which means that something like
[prop]="'{{ foo }}'"
will be considered an error, even though it's not actually an interpolation.These changes build on top of the logic from #39826 to account for interpolation characters inside quotes.
Fixes #39601.