Support two-way bindings in Ivy native language service #40185
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pullapprove
bot
added
area: compiler
|
|
atscott
force-pushed
the
bananabox
branch
2 times, most recently
from
2ea1eff
to
23b90dd
Compare
alxhub
reviewed
Jan 6, 2021
alxhub
reviewed
Jan 6, 2021
alxhub
approved these changes
Jan 6, 2021
…y binding This commit fixes the Template Type Checker's `getSymbolOfNode` so that it is able to retrieve a symbol for the `BoundEvent` of a two-way binding. Previously, the implementation would locate the node in the TCB for the input because it appeared first and shares the same `keySpan` as the event binding. To fix this, the TCB node search now verifies that the located node matches the expected name for the output subscription: either `addEventListener` for a native listener or the class member of the Angular `@Output` in the case of an Angular output, as would be the case for two-way bindings.
atscott
force-pushed
the
bananabox
branch
2 times, most recently
from
b4d141d
to
2b3181e
Compare
atscott
requested review from
kyliau and
AndrewKushnir
and removed request for
AndrewKushnir
kyliau
approved these changes
Jan 7, 2021
| */ | ||
| export interface TwoWayBindingContext { | ||
| kind: TargetNodeKind.TwoWayBindingContext; | ||
| nodes: [t.BoundAttribute, t.BoundEvent]; |
There was a problem hiding this comment.
nit: Consider using explicit fields like input and output instead of a tuple.
There was a problem hiding this comment.
@alxhub and I talked about this and kind of liked that if we find more MultiNodeTarget types at some point, there would still be the consistent nodes property.
…icate multi-node targets The current template target implementation only allows a way to represent the template position as targeting a single node in the template AST. However, there is at least one case (banana-in-a-box) where a given template position refers to two template targets. This commit expands the contexts that the `TemplateTarget` can return to include support for the banana-in-a-box syntax, which has two logically targetted AST nodes given a position within the `keySpan` of the binding.
…dings Adjust the visitor logic of the template target as well as the consumption of the visitor result to account for two-way bindings. This sets up downstream consumers for being able to handle the possibility of a template position that targets both an input and an output.
Rather than expecting that a position in a template only targets a single node, this commit adjusts the approach to account for two way bindings. In particular, we attempt to get definitions for each targeted node and then return the combination of all results, or `undefined` if none of the target nodes had definitions.
Rather than expecting that a position in a template only targets a single node, this commit simply adjusts the approach to account for two way bindings. Specifically, we attempt to get references for each targeted node and then return the combination of all results, or `undefined` if none of the target nodes had references.
This commit adds special handling to the completion builder by detecting a two way binding context and ensuring that we filter out any `Input`s that do not support two way binding.
…two-way bindings This commit simply adds a comment about why quick info only gets data for the `BoundAttribute` of a two-way binding.
atscott
added
action: merge
atscott
added a commit
that referenced
this pull request
Jan 7, 2021
…icate multi-node targets (#40185) The current template target implementation only allows a way to represent the template position as targeting a single node in the template AST. However, there is at least one case (banana-in-a-box) where a given template position refers to two template targets. This commit expands the contexts that the `TemplateTarget` can return to include support for the banana-in-a-box syntax, which has two logically targetted AST nodes given a position within the `keySpan` of the binding. PR Close #40185
atscott
added a commit
that referenced
this pull request
Jan 7, 2021
…dings (#40185) Adjust the visitor logic of the template target as well as the consumption of the visitor result to account for two-way bindings. This sets up downstream consumers for being able to handle the possibility of a template position that targets both an input and an output. PR Close #40185
atscott
added a commit
that referenced
this pull request
Jan 7, 2021
#40185) Rather than expecting that a position in a template only targets a single node, this commit adjusts the approach to account for two way bindings. In particular, we attempt to get definitions for each targeted node and then return the combination of all results, or `undefined` if none of the target nodes had definitions. PR Close #40185
atscott
added a commit
that referenced
this pull request
Jan 7, 2021
…#40185) Rather than expecting that a position in a template only targets a single node, this commit simply adjusts the approach to account for two way bindings. Specifically, we attempt to get references for each targeted node and then return the combination of all results, or `undefined` if none of the target nodes had references. PR Close #40185
atscott
added a commit
that referenced
this pull request
Jan 7, 2021
This commit adds special handling to the completion builder by detecting a two way binding context and ensuring that we filter out any `Input`s that do not support two way binding. PR Close #40185
|
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. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adjusts various pieces of the Ivy native LS implementation to account for two-way bindings (aka banana in a box syntax).
Each commit has a very targeted goal to make it easier to review incrementally.