fix($interpolate): use the first end symbol that forms a valid expression #8744
Conversation
|
Then we spend potentially a lot more time parsing =\ also try/catch blocks --- it looks like it will prevent us from throwing in the case of invalid expressions, am i wrong? |
|
Updated the PR with another approach that keeps track of |
| break; | ||
| } | ||
| index++; | ||
| } |
There was a problem hiding this comment.
This was kind of a hard read, I think I would refactor this to something like:
var brackets = [[0, 0, 0], [0, 0, 0]], bracketChars = '({[)}]', ch, quoteCh;
index = startIndex + startSymbolLength;
while (index !== endIndex || quoteCh || !equals(brackets[0], brackets[1])) {
ch = text[index];
if (quoteCh) {
if (ch === '\\') {
index++;
} else if (ch === quoteCh) {
quoteCh = undefined;
}
} else if (ch === "'" || ch === '"') {
quoteCh = ch;
} else if ((i = bracketChars.indexOf(ch)) != -1) {
brackets[i/3][i%3]++;
}
index++;
if (index > endIndex) {
endIndex = text.indexOf(endSymbol, index);
if (endIndex === -1) {
break;
}
}
}(haven't tested, I might have some bug there)
There was a problem hiding this comment.
The suggested code will fail with $interpolate('{{"\\}}"}}'), anyhow it is possible to fix it, but it does add some other oddities
There was a problem hiding this comment.
As I said, that's just a suggestion for a direction to get rid of this pretty cryptic switch statement, I didn't really test it. Anyway, I modified it a bit so it will work with the failure you mentioned.
|
I think I like the try/catch better since my guess is that this implementation is much less performant in "sunny day" cases (where the expression does not contain }}). |
|
The try/catch case is broken because it causes us to silently fail when there are legitimate syntax errors. I don't really expect this bug to actually get fixed, since it's been around for so long and people rarely get bit by it |
|
Not sure what you mean, looking at lgalfaso@2c64dc4 it seems to me that it will re-throw the last exception when it runs out of endSymbols ahead. Regarding the chances of this getting fixed, you're probably right :) |
|
The PR is in place to start the discussion on if this should be fixed and if that is the case, the solution that should be implemented |
|
@caitp not sure why you say that the PR silently fails on syntax errors, it will still throw when these are found |
…sion When an interpolation string has multiple end symbol choices, pick the occurrence that forms a valid expression Closes angular#8642
|
I saw that it rethrows the second time I looked at it, but I still wouldn't expect it to land |
petebacondarwin
force-pushed
the
master
branch
from
02dc2aa to
fd2d6c0
Compare
jeffbcross
force-pushed
the
master
branch
2 times, most recently
from
cad9560 to
f294244
Compare
jeffbcross
force-pushed
the
master
branch
4 times, most recently
from
e8dc429 to
e83fab9
Compare
petebacondarwin
force-pushed
the
master
branch
from
4dd5a20 to
998c61c
Compare
When an interpolation string has multiple end symbol choices, pick the
occurrence that forms a valid expression
Closes #8642