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

refactor(core): Consolidates shouldTrigger* methods down to one #58833

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

thePunderWoman
Copy link
Contributor

This removes potential confusion by relying on one shouldTriggerDeferBlock method everywhere.

PR Type

What kind of change does this PR introduce?

  • Refactoring (no functional changes, no api changes)

Does this PR introduce a breaking change?

  • Yes
  • No

@thePunderWoman thePunderWoman added action: review The PR is still awaiting reviews from at least one requested reviewer area: core Issues related to the framework runtime target: patch This PR is targeted for the next patch release labels Nov 22, 2024
@ngbot ngbot bot added this to the Backlog milestone Nov 22, 2024
@angular-robot angular-robot bot added area: core Issues related to the framework runtime and removed area: core Issues related to the framework runtime labels Nov 22, 2024
@ngbot ngbot bot modified the milestone: Backlog Nov 22, 2024
This removes potential confusion by relying on one shouldTriggerDeferBlock method everywhere.
@angular-robot angular-robot bot added area: core Issues related to the framework runtime and removed area: core Issues related to the framework runtime labels Nov 22, 2024
@ngbot ngbot bot modified the milestone: Backlog Nov 22, 2024
Comment on lines 182 to +186
export function ɵɵdeferWhen(rawValue: unknown) {
const lView = getLView();
const tNode = getSelectedTNode();

if (!shouldTriggerDeferBlock(lView, tNode)) return;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to propose a slightly different API. We can have a function that would accept the necessary details (e.g. lView, tNode, etc) and also a trigger type and return whether those triggers should be attached, for example:

export function shouldAttachTrigger(triggerType: TriggerType, lView: LView, tNode: TNode): boolean {
  // ... determine whether a given trigger type should be activated ...
}

The actual usage would look like this:

export function ɵɵdeferWhen(rawValue: unknown) {
  const lView = getLView();
  const tNode = getSelectedTNode();

  if (shouldAttachTrigger(TriggerType.Regular, lView, tNode)) {
    // ... the code that attaches the trigger ...
  }

For a "hydrate" trigger:

export function ɵɵdeferHydrateWhen(rawValue: unknown) {
  const lView = getLView();
  const tNode = getSelectedTNode();

  if (shouldAttachTrigger(TriggerType.Hydrate, lView, tNode)) {
    // ... the code that attaches the trigger ...
  }

We'd need to ensure that the TriggerType.Regular and TriggerType.Hydrate are mutually exclusive, i.e. we can compute a value based on the rules mentioned in the code below and return true/false depending on a trigger type.

@@ -342,19 +341,14 @@ export function ɵɵdeferHydrateOnIdle() {
export function ɵɵdeferOnImmediate() {
const lView = getLView();
const tNode = getCurrentTNode()!;
const tView = lView[TVIEW];
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 function is missing an early exit (similar to other functions).

Comment on lines +77 to +78
const isServerRendered =
lDetails[SSR_BLOCK_STATE] !== null && lDetails[SSR_BLOCK_STATE] === DeferBlockState.Complete;
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess the intention was to check for the SSR_UNIQUE_ID field value presence?

Suggested change
const isServerRendered =
lDetails[SSR_BLOCK_STATE] !== null && lDetails[SSR_BLOCK_STATE] === DeferBlockState.Complete;
const isServerRendered =
lDetails[SSR_UNIQUE_ID] !== null && lDetails[SSR_BLOCK_STATE] === DeferBlockState.Complete;

Copy link
Contributor

Choose a reason for hiding this comment

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

I also think that there should be no conditions in this function and we should move the check to an instruction level (i.e. to a call site).

Comment on lines +498 to +500
const hasHydrateTriggers =
lDetails[SSR_UNIQUE_ID] !== null ||
(tDetails.hydrateTriggers !== null && tDetails.hydrateTriggers.size > 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

If we rely on the tDetails.hydrateTriggers presence, we'll make an implicit ordering dependency between deferHydrate* and defer* (non-hydrate) instructions, i.e. hydrate ones must be invoked first. This might be a problem for deferHydrateWhen instruction, which would end up in the "update" block of a template function, when the other functions (e.g. deferOnIdle) would be in the "create" block. I'm wondering if actually checking the SSR_UNIQUE_ID field value presence in LDetails should be sufficient. That would mean that this block was triggered on the server, thus it has "hydrate" triggers.

Suggested change
const hasHydrateTriggers =
lDetails[SSR_UNIQUE_ID] !== null ||
(tDetails.hydrateTriggers !== null && tDetails.hydrateTriggers.size > 0);
const hasHydrateTriggers = lDetails[SSR_UNIQUE_ID] !== null;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action: review The PR is still awaiting reviews from at least one requested reviewer area: core Issues related to the framework runtime target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants