feat(compiler): Add sourceSpan and keySpan to TemplateBinding #35897
Conversation
kyliau
added
area: compiler
kyliau
force-pushed
the
key-value-span-2
branch
3 times, most recently
from
b0431bd
to
d2c1044
Compare
There was a problem hiding this comment.
Thanks for the changes and additional docs @kyliau 👍
I've added a couple comments, could you please have a look?
Thank you.
kyliau
force-pushed
the
key-value-span-2
branch
2 times, most recently
from
8efd373
to
6958f69
Compare
There was a problem hiding this comment.
@AndrewKushnir @ayazhafiz Thank you for the valuable feedback!
After analyzing the structure of the code, I realized much of the complexity comes from the decision to use ASTWithSource to represent the value of the binding for both variable binding and expression binding.
ASTWithSource is messy to work with because it contains both relative span and absolute span, so we had to keep track of both.
The fundamental problem stems from the usage of field variable keyIsVar to differentiate the data types for variable binding and expression binding. The right thing to do here is to use the type information to represent the data, and I've created two separate classes, VariableBinding and ExpressionBinding to reflect this.
With this change, there is no more confusing relative spans to deal with. All spans handled within the microsyntax expression parsing are absolute spans. The implementation should be much cleaner now. Please take a look and let me know what you think. Thank you!
kyliau
force-pushed
the
key-value-span-2
branch
3 times, most recently
from
714ff0a
to
ac8dd2c
Compare
|
@ayazhafiz @AndrewKushnir I've addressed all comments, could you please take another look? Thank you! |
There was a problem hiding this comment.
LGTM, thanks @kyliau 👍
I left a few comments (mostly docs and minor refactoring proposals), please have a look when you get a chance.
Thank you.
|
@kyliau I think it'd make sense to also run a Global TAP (in addition to Blueprint one) to make sure everything looks good. Thank you. |
This commit adds fine-grained text spans to TemplateBinding for microsyntax expressions. 1. Source span By convention, source span refers to the entire span of the binding, including its key and value. 2. Key span Span of the binding key, without any whitespace or keywords like `let` The value span is captured by the value expression AST. This is part of a series of PRs to fix source span mapping in microsyntax expression. For more info, see the doc https://docs.google.com/document/d/1mEVF2pSSMSnOloqOPQTYNiAJO0XQxA1H0BZyESASOrE/edit?usp=sharing
kyliau
force-pushed
the
key-value-span-2
branch
from
ac8dd2c
to
5af53ce
Compare
My intention was to separate the testing for key value and their spans, so that the tests are focused. There is a dedicated section for testing spans only from line 482-552. |
matsko
added
target: major
…r#35897) This commit adds fine-grained text spans to TemplateBinding for microsyntax expressions. 1. Source span By convention, source span refers to the entire span of the binding, including its key and value. 2. Key span Span of the binding key, without any whitespace or keywords like `let` The value span is captured by the value expression AST. This is part of a series of PRs to fix source span mapping in microsyntax expression. For more info, see the doc https://docs.google.com/document/d/1mEVF2pSSMSnOloqOPQTYNiAJO0XQxA1H0BZyESASOrE/edit?usp=sharing PR Close angular#35897 (cherry picked from commit 06779cf)
|
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. |
This commit adds fine-grained text spans to TemplateBinding for microsyntax expressions.
By convention, source span refers to the entire span of the binding,
including its key and value.
Span of the binding key, without any whitespace or keywords like
letThe value span is captured by the value expression AST.
These spans are primarily needed by the language service to provide precise autocomplete
suggestions and quick hover info at particular position.
The tests for microsyntax expressions are totally overhauled for better coverage, conciseness, and organization.
This is part of a series of PRs to fix source span mapping in microsyntax expression.
For more info, please see the design doc https://docs.google.com/document/d/1mEVF2pSSMSnOloqOPQTYNiAJO0XQxA1H0BZyESASOrE/edit?usp=sharing
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information