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

Implement deferred interaction triggers #51830

Closed
wants to merge 3 commits into from

Conversation

crisbeto
Copy link
Member

@crisbeto crisbeto commented Sep 20, 2023

Includes a few prerequisites, as well as the actual implementation, for the on interaction and prefetch on interaction triggers. The changes here will be the base for implementing the on viewport and on hover triggers as well. Includes the following commits:

refactor(compiler): extract deferred block trigger information

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.

refactor(compiler): implement final instruction generation for interaction triggers

Updates the logic that generates the instructions for the on interaction and prefetch on interaction triggers to their final shape. Now the instructions take two arguments:

  1. triggerIndex - index at which to find the trigger in the view where it will be rendered.
  2. walkUpTimes - tells the runtime how many views up it needs to go to find the trigger element. If the argument is omitted, it means that the trigger is in the same view as the deferred block. A positive number means that the runtime needs to go up X amount of times to find the trigger. A negative number means that the trigger is inside the root view of the placeholder block. Negative numbers are capped at -1 since the placeholder is always in the same position at runtime.

feat(core): implement deferred block interaction triggers

Adds the implementation for the on interaction and prefetch on interaction triggers.

@crisbeto crisbeto added action: review The PR is still awaiting reviews from at least one requested reviewer area: core Issues related to the framework runtime target: major This PR is targeted for the next major release labels Sep 20, 2023
@ngbot ngbot bot modified the milestone: Backlog Sep 20, 2023
@angular-robot angular-robot bot added the detected: feature PR contains a feature commit label Sep 20, 2023
/** Manages events related to deferred blocks. */
export class ɵDeferEventManager {
/** @nocollapse */
static ɵprov = /** @pureOrBreakMyCode */ ɵɵdefineInjectable({
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 made this class an injectable, because we'll need access to NgZone for some of the other triggers.

});

describe('interaction triggers', () => {
it('should load the deferred content when the trigger is clicked', fakeAsync(() => {
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 had to use fakeAsync here, because I couldn't get the afterRender to fire predictably in the browser tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be possible to repro this in main using afterRender and a similar component structure? It'd be interesting to investigate to make sure there are no issues with afterRender timing.

Copy link
Member Author

Choose a reason for hiding this comment

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

From my investigation, afterRender was definitely firing. The problem was that it's asynchronous and we need it to fire before the relevant assertion. I'm not sure that the defer tests should be concerned with the afterRender timing. That's why afterRender has tests of its own.

@crisbeto crisbeto marked this pull request as ready for review September 20, 2023 10:07
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! I've left a few comments. I think we'd need to update how we keep track of prefetching triggers before we merge the PR.

const tDetails = getTDeferBlockDetails(tView, tNode);

if (tDetails.loadingState === DeferDependenciesLoadingState.NOT_STARTED) {
tDetails.loadingState = DeferDependenciesLoadingState.SCHEDULED;
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this logic is similar to the one in ɵɵdeferPrefetchOnIdle, but I think it would not work in both places. If we have both prefetch on idle; prefetch on interaction(...) defined, the interaction one would not never be invoked (we'll never make it through the if statement), which is incorrect. We need a different tracking mechanism: something like a WeakSet in tDetails, which would contain keys that uniquely identify those triggers, like this:

const trigger = DeferBlockTriggers.OnInteraction; // const enum with all triggers
const key = `${trigger}|${triggerIndex}|${walkUpTimes ?? 0}`;
const scheduledTriggers = tDetails.scheduledTriggers;
if (tDetails.loadingState === DeferDependenciesLoadingState.NOT_STARTED && !scheduledTriggers.has(key)) {
  scheduledTriggers.add(key);
  onInteraction(
        lView, tNode, triggerIndex, walkUpTimes, () => triggerPrefetching(tDetails, lView));
}

This would only be needed though for cases when we use local ref from a template. If a local ref comes from placeholder - we should probably keep scheduling callbacks, since trigger elements would be different for each defer block.

We'll need to do similar change for on idle condition and drop the SCHEDULED state.

WDYT?

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'm not sure I understand what the issue is here. Aren't both triggers trying to start the preloading of the same resources? Hence we care until one of them fires first after which we can cancel the rest of the triggers. We can move this check inside the afterRender to avoid binding any events if on idle started it already.

Copy link
Contributor

Choose a reason for hiding this comment

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

This logic schedules resource loading operation, but doesn't actually initiate the loading process. As a result, there might be multiple prefetch triggers that would result in scheduling multiple operations. Once the first one is triggered, we should cleanup the rest. In some cases (e.g. on idle or when we do on interaction(localRefFromTemplate)), we do not want to schedule prefetching more than once per defer block type (that may have multiple instances, for ex. if a defer is in a for loop).

I've put together a PR with a proposed change to unify the management of prefetch triggers and cleanup functions: #51856. Please let me know what you think about it.

packages/core/src/render3/instructions/defer.ts Outdated Show resolved Hide resolved
});

describe('interaction triggers', () => {
it('should load the deferred content when the trigger is clicked', fakeAsync(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be possible to repro this in main using afterRender and a similar component structure? It'd be interesting to investigate to make sure there are no issues with afterRender timing.

@crisbeto
Copy link
Member Author

I've addressed all of the feedback, except the one that depends on #51856. I also ported over some of the changes from my on hover and on viewport branch.

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`.
…ction triggers

Updates the logic that generates the instructions for the `on interaction` and `prefetch on interaction` triggers to their final shape. Now the instructions take two arguments:
1. `triggerIndex` - index at which to find the trigger in the view where it will be rendered.
2. `walkUpTimes` - tells the runtime how many views up it needs to go to find the trigger element. If the argument is omitted, it means that the trigger is in the same view as the deferred block. A positive number means that the runtime needs to go up X amount of times to find the trigger. A negative number means that the trigger is inside the root view of the placeholder block. Negative numbers are capped at -1 since the placeholder is always in the same position at runtime.
Adds the implementation for the `on interaction` and `prefetch on interaction` triggers.
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.

👍

@AndrewKushnir AndrewKushnir added action: presubmit The PR is in need of a google3 presubmit and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Sep 22, 2023
@crisbeto crisbeto added action: merge The PR is ready for merge by the caretaker and removed action: presubmit The PR is in need of a google3 presubmit labels Sep 22, 2023
@dylhunn
Copy link
Contributor

dylhunn commented Sep 22, 2023

This PR was merged into the repository by commit 3cbb2a8.

@dylhunn dylhunn closed this in 7cce28a Sep 22, 2023
dylhunn pushed a commit that referenced this pull request Sep 22, 2023
…ction triggers (#51830)

Updates the logic that generates the instructions for the `on interaction` and `prefetch on interaction` triggers to their final shape. Now the instructions take two arguments:
1. `triggerIndex` - index at which to find the trigger in the view where it will be rendered.
2. `walkUpTimes` - tells the runtime how many views up it needs to go to find the trigger element. If the argument is omitted, it means that the trigger is in the same view as the deferred block. A positive number means that the runtime needs to go up X amount of times to find the trigger. A negative number means that the trigger is inside the root view of the placeholder block. Negative numbers are capped at -1 since the placeholder is always in the same position at runtime.

PR Close #51830
dylhunn pushed a commit that referenced this pull request Sep 22, 2023
Adds the implementation for the `on interaction` and `prefetch on interaction` triggers.

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 23, 2023
ChellappanRajan pushed a commit to ChellappanRajan/angular that referenced this pull request Jan 23, 2024
…ar#51830)

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`.

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

Updates the logic that generates the instructions for the `on interaction` and `prefetch on interaction` triggers to their final shape. Now the instructions take two arguments:
1. `triggerIndex` - index at which to find the trigger in the view where it will be rendered.
2. `walkUpTimes` - tells the runtime how many views up it needs to go to find the trigger element. If the argument is omitted, it means that the trigger is in the same view as the deferred block. A positive number means that the runtime needs to go up X amount of times to find the trigger. A negative number means that the trigger is inside the root view of the placeholder block. Negative numbers are capped at -1 since the placeholder is always in the same position at runtime.

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

Adds the implementation for the `on interaction` and `prefetch on interaction` triggers.

PR Close angular#51830
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: core Issues related to the framework runtime detected: feature PR contains a feature commit 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