Skip to content
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

Deferred trigger prerequisite refactors #51816

Closed
wants to merge 3 commits into from

Conversation

crisbeto
Copy link
Member

These changes include some refactors to facilitate the implementation of the viewport, interaction and hover triggers. The majority of the changes is around reworking the R3TargetBinder to consider if, switch and defer blocks (plus the sub-blocks) as separate embedded views. This allows us to properly resolve references within their scope and is consistent with how they behave at runtime. Split up across the following commits to make it easier to review:

refactor(compiler): require a reference in interaction and hover triggers

Updates the parsing for interaction and hover triggers to require a reference to an element.

refactor(compiler): update binder to account for new semantics

When the TargetBinder was written, the only embedded-view-based nodes were templates, but now we have {#if}, {#switch} and {#defer} which have similar semantics. These changes rework the binder to account for the new nodes.

refactor(compiler): add utility to resolve the deferred block trigger element

Adds a utility to the BoundTarget that helps with resolving which element a deferred block is pointing to. We need a separate method for this, because deferred blocks have some special logic for where the trigger can be located.

…gers

Updates the parsing for `interaction` and `hover` triggers to require a reference to an element.
When the `TargetBinder` was written, the only embedded-view-based nodes were templates, but now we have `{#if}`, `{#switch}` and `{#defer}` which have similar semantics. These changes rework the binder to account for the new nodes.
… element

Adds a utility to the `BoundTarget` that helps with resolving which element a deferred block is pointing to. We need a separate method for this, because deferred blocks have some special logic for where the trigger can be located.
@crisbeto crisbeto added action: review The PR is still awaiting reviews from at least one requested reviewer target: major This PR is targeted for the next major release area: compiler Issues related to `ngc`, Angular's template compiler labels Sep 18, 2023
@ngbot ngbot bot added this to the Backlog milestone Sep 18, 2023
@crisbeto crisbeto marked this pull request as ready for review September 18, 2023 20:11
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@crisbeto looks great 👍

// Use the `Scope` to extract the entities present at every level of the template.
const templateEntities = extractTemplateEntities(scope);
const scopedNodeEntities = extractScopedNodeEntities(scope);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

Suggested change
const scopedNodeEntities = extractScopedNodeEntities(scope);
const scopedNodeEntities = extractScopedNodes(scope);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the current name is more accurate, because we aren't extracting the scoped nodes themselves, but rather entities (e.g. Reference and Variable) present in them.

@@ -54,7 +53,7 @@ export class R3TargetBinder<DirectiveT extends DirectiveMeta> implements TargetB
TemplateBinder.applyWithScope(target.template, scope);
return new R3BoundTarget(
target, directives, eagerDirectives, bindings, references, expressions, symbols,
nestingLevel, templateEntities, usedPipes, eagerPipes, deferBlocks);
nestingLevel, scopedNodeEntities, usedPipes, eagerPipes, deferBlocks);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
nestingLevel, scopedNodeEntities, usedPipes, eagerPipes, deferBlocks);
nestingLevel, scopedNodes, usedPipes, eagerPipes, deferBlocks);

Comment on lines +548 to +551
} else if (
nodeOrNodes instanceof DeferredBlock || nodeOrNodes instanceof DeferredBlockError ||
nodeOrNodes instanceof DeferredBlockPlaceholder ||
nodeOrNodes instanceof DeferredBlockLoading) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we use this check in a couple places, it'd be great to extract it to a separate fn:

Suggested change
} else if (
nodeOrNodes instanceof DeferredBlock || nodeOrNodes instanceof DeferredBlockError ||
nodeOrNodes instanceof DeferredBlockPlaceholder ||
nodeOrNodes instanceof DeferredBlockLoading) {
} else if (isScopedNode(nodeOrNodes)) {

We can check all node types there (including IfBlockBranch and ForLoopBlock), so we can do do type narrowing too:

function isScopedNode(node: unknown): node is ScopedNode { ... }

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that this felt a bit weird when I was writing it, but there are a couple of reasons why I think we should keep it:

  1. Once template type checking is implemented for control flow, there will be different handling for each ScopedNode, with the exception of the deferred ones.
  2. Using the if/else if like we do here will result in an error whenever we add a node type to ScopedNode, which is good because it makes it easier to see where we need to adjust the logic. In comparison, if we do something like following we won't get such an error:
if (!isScopedNode(node)) {
  return ...;
}

if (node instanceof IfBlock) {
   ...
} else if (node instanceof DeferredBlock) {
   ...
}

@AndrewKushnir AndrewKushnir removed the action: review The PR is still awaiting reviews from at least one requested reviewer label Sep 19, 2023
@crisbeto crisbeto added the action: merge The PR is ready for merge by the caretaker label Sep 19, 2023
@pkozlowski-opensource
Copy link
Member

This PR was merged into the repository by commit d538908.

pkozlowski-opensource pushed a commit that referenced this pull request Sep 19, 2023
When the `TargetBinder` was written, the only embedded-view-based nodes were templates, but now we have `{#if}`, `{#switch}` and `{#defer}` which have similar semantics. These changes rework the binder to account for the new nodes.

PR Close #51816
pkozlowski-opensource pushed a commit that referenced this pull request Sep 19, 2023
… element (#51816)

Adds a utility to the `BoundTarget` that helps with resolving which element a deferred block is pointing to. We need a separate method for this, because deferred blocks have some special logic for where the trigger can be located.

PR Close #51816
crisbeto added a commit to crisbeto/angular that referenced this pull request Sep 20, 2023
Reworks the compiler to use the API introduced in angular#51816 to match triggers to the element nodes they point to. This will be used to generate the new instructions for `on interaction` and `prefetch on interaction`.
crisbeto added a commit to crisbeto/angular that referenced this pull request Sep 20, 2023
Reworks the compiler to use the API introduced in angular#51816 to match triggers to the element nodes they point to. This will be used to generate the new instructions for `on interaction` and `prefetch on interaction`.
crisbeto added a commit to crisbeto/angular that referenced this pull request Sep 22, 2023
Reworks the compiler to use the API introduced in angular#51816 to match triggers to the element nodes they point to. This will be used to generate the new instructions for `on interaction` and `prefetch on interaction`.
crisbeto added a commit to crisbeto/angular that referenced this pull request Sep 22, 2023
Reworks the compiler to use the API introduced in angular#51816 to match triggers to the element nodes they point to. This will be used to generate the new instructions for `on interaction` and `prefetch on interaction`.
dylhunn pushed a commit that referenced this pull request Sep 22, 2023
Reworks the compiler to use the API introduced in #51816 to match triggers to the element nodes they point to. This will be used to generate the new instructions for `on interaction` and `prefetch on interaction`.

PR Close #51830
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Oct 20, 2023
ChellappanRajan pushed a commit to ChellappanRajan/angular that referenced this pull request Jan 23, 2024
…gers (angular#51816)

Updates the parsing for `interaction` and `hover` triggers to require a reference to an element.

PR Close angular#51816
ChellappanRajan pushed a commit to ChellappanRajan/angular that referenced this pull request Jan 23, 2024
…ar#51816)

When the `TargetBinder` was written, the only embedded-view-based nodes were templates, but now we have `{#if}`, `{#switch}` and `{#defer}` which have similar semantics. These changes rework the binder to account for the new nodes.

PR Close angular#51816
ChellappanRajan pushed a commit to ChellappanRajan/angular that referenced this pull request Jan 23, 2024
… element (angular#51816)

Adds a utility to the `BoundTarget` that helps with resolving which element a deferred block is pointing to. We need a separate method for this, because deferred blocks have some special logic for where the trigger can be located.

PR Close angular#51816
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker area: compiler Issues related to `ngc`, Angular's template compiler target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants