-
Notifications
You must be signed in to change notification settings - Fork 25.5k
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
base: main
Are you sure you want to change the base?
Conversation
This removes potential confusion by relying on one shouldTriggerDeferBlock method everywhere.
48cb799
to
1be4c04
Compare
export function ɵɵdeferWhen(rawValue: unknown) { | ||
const lView = getLView(); | ||
const tNode = getSelectedTNode(); | ||
|
||
if (!shouldTriggerDeferBlock(lView, tNode)) return; |
There was a problem hiding this comment.
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]; |
There was a problem hiding this comment.
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).
const isServerRendered = | ||
lDetails[SSR_BLOCK_STATE] !== null && lDetails[SSR_BLOCK_STATE] === DeferBlockState.Complete; |
There was a problem hiding this comment.
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?
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; |
There was a problem hiding this comment.
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).
const hasHydrateTriggers = | ||
lDetails[SSR_UNIQUE_ID] !== null || | ||
(tDetails.hydrateTriggers !== null && tDetails.hydrateTriggers.size > 0); |
There was a problem hiding this comment.
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.
const hasHydrateTriggers = | |
lDetails[SSR_UNIQUE_ID] !== null || | |
(tDetails.hydrateTriggers !== null && tDetails.hydrateTriggers.size > 0); | |
const hasHydrateTriggers = lDetails[SSR_UNIQUE_ID] !== null; |
This removes potential confusion by relying on one shouldTriggerDeferBlock method everywhere.
PR Type
What kind of change does this PR introduce?
Does this PR introduce a breaking change?