From cdf1ea1951fb7187b1f6c9bb8a847c859c41e0b8 Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Wed, 17 Feb 2021 11:23:18 +0100 Subject: [PATCH] refactor(compiler): retrieve variables from context inside nested template listener (#40833) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is a pre-requisite for #40360. Given the following template which has a listener that references a variable from a parent template (`name`): ``` ``` We generate code that looks that looks like. Note how we access `name` through `ctx`: ```js function template(rf, ctx) { if (rf & 1) { const r0 = ɵɵgetCurrentView(); ɵɵelementStart(0, "button", 2); ɵɵlistener("click", function() { ɵɵrestoreView(r0); const name_r0 = ctx.name; // Note the `ctx.name` access here. const ctx_r1 = ɵɵnextContext(); return ctx_r1.log(name_r0); }); ɵɵelementEnd(); } } ``` This works fine at the moment, because the template context object can't be changed after creation. The changes in #40360 allow for the object to be changed, which means that the `ctx` reference inside the listener will be out of date, because it was bound during creation mode. This PR aims to address the issue by accessing the context inside listeners through the saved view reference. With the new code, the generated code from above will look as follows: ```js function template(rf, ctx) { if (rf & 1) { const r0 = ɵɵgetCurrentView(); ɵɵelementStart(0, "button", 2); ɵɵlistener("click", function() { const restoredCtx = ɵɵrestoreView(r0); const name_r0 = restoredCtx.name; const ctx_r1 = ɵɵnextContext(); return ctx_r1.log(name_r0); }); ɵɵelementEnd(); } } ``` PR Close #40833 --- ...template_context_many_bindings_template.js | 6 +-- .../nested_template_context_template.js | 4 +- .../r3_view_compiler_template_spec.ts | 10 ++-- .../compiler/src/render3/view/template.ts | 46 +++++++++++++++---- packages/compiler/src/render3/view/util.ts | 3 ++ packages/core/src/render3/state.ts | 4 +- 6 files changed, 52 insertions(+), 21 deletions(-) diff --git a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_template/nested_template_context_many_bindings_template.js b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_template/nested_template_context_many_bindings_template.js index 4a6ef26552..84bc2ae9d9 100644 --- a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_template/nested_template_context_many_bindings_template.js +++ b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_template/nested_template_context_many_bindings_template.js @@ -3,9 +3,9 @@ function MyComponent_div_0_Template(rf, ctx) { const $s$ = $r3$.ɵɵgetCurrentView(); $r3$.ɵɵelementStart(0, "div", 1); $r3$.ɵɵlistener("click", function MyComponent_div_0_Template_div_click_0_listener() { - $r3$.ɵɵrestoreView($s$); - const $d$ = ctx.$implicit; - const $i$ = ctx.index; + const $sr$ = $r3$.ɵɵrestoreView($s$); + const $d$ = $sr$.$implicit; + const $i$ = $sr$.index; const $comp$ = $r3$.ɵɵnextContext(); return $comp$._handleClick($d$, $i$); }); diff --git a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_template/nested_template_context_template.js b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_template/nested_template_context_template.js index 2665d80207..aba3ec7d15 100644 --- a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_template/nested_template_context_template.js +++ b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_template/nested_template_context_template.js @@ -3,8 +3,8 @@ function MyComponent_ul_0_li_1_div_1_Template(rf, ctx) { const $s$ = $i0$.ɵɵgetCurrentView(); $i0$.ɵɵelementStart(0, "div", 2); $i0$.ɵɵlistener("click", function MyComponent_ul_0_li_1_div_1_Template_div_click_0_listener(){ - $i0$.ɵɵrestoreView($s$); - const $inner$ = ctx.$implicit; + const $sr$ = $i0$.ɵɵrestoreView($s$); + const $inner$ = $sr$.$implicit; const $middle$ = $i0$.ɵɵnextContext().$implicit; const $outer$ = $i0$.ɵɵnextContext().$implicit; const $myComp$ = $i0$.ɵɵnextContext(); diff --git a/packages/compiler-cli/test/compliance_old/r3_view_compiler_template_spec.ts b/packages/compiler-cli/test/compliance_old/r3_view_compiler_template_spec.ts index 7e1895794a..26fd0703e4 100644 --- a/packages/compiler-cli/test/compliance_old/r3_view_compiler_template_spec.ts +++ b/packages/compiler-cli/test/compliance_old/r3_view_compiler_template_spec.ts @@ -55,8 +55,8 @@ describe('compiler compliance: template', () => { const $s$ = $i0$.ɵɵgetCurrentView(); $i0$.ɵɵelementStart(0, "div", 2); $i0$.ɵɵlistener("click", function MyComponent_ul_0_li_1_div_1_Template_div_click_0_listener(){ - $i0$.ɵɵrestoreView($s$); - const $inner$ = ctx.$implicit; + const $sr$ = $i0$.ɵɵrestoreView($s$); + const $inner$ = $sr$.$implicit; const $middle$ = $i0$.ɵɵnextContext().$implicit; const $outer$ = $i0$.ɵɵnextContext().$implicit; const $myComp$ = $i0$.ɵɵnextContext(); @@ -150,9 +150,9 @@ describe('compiler compliance: template', () => { const $s$ = $r3$.ɵɵgetCurrentView(); $r3$.ɵɵelementStart(0, "div", 1); $r3$.ɵɵlistener("click", function MyComponent_div_0_Template_div_click_0_listener() { - $r3$.ɵɵrestoreView($s$); - const $d$ = ctx.$implicit; - const $i$ = ctx.index; + const $sr$ = $r3$.ɵɵrestoreView($s$); + const $d$ = $sr$.$implicit; + const $i$ = $sr$.index; const $comp$ = $r3$.ɵɵnextContext(); return $comp$._handleClick($d$, $i$); }); diff --git a/packages/compiler/src/render3/view/template.ts b/packages/compiler/src/render3/view/template.ts index e7d7618cc8..88ad359364 100644 --- a/packages/compiler/src/render3/view/template.ts +++ b/packages/compiler/src/render3/view/template.ts @@ -39,7 +39,7 @@ import {createLocalizeStatements} from './i18n/localize_utils'; import {I18nMetaVisitor} from './i18n/meta'; import {assembleBoundTextPlaceholders, assembleI18nBoundString, declareI18nVariable, getTranslationConstPrefix, hasI18nMeta, I18N_ICU_MAPPING_PREFIX, i18nFormatPlaceholderNames, icuFromI18nMessage, isI18nRootNode, isSingleI18nIcu, placeholdersToParams, TRANSLATION_VAR_PREFIX, wrapI18nPlaceholder} from './i18n/util'; import {StylingBuilder, StylingInstruction} from './styling_builder'; -import {asLiteral, chainedInstruction, CONTEXT_NAME, getAttrsForDirectiveMatching, getInterpolationArgsLength, IMPLICIT_REFERENCE, invalid, NON_BINDABLE_ATTR, REFERENCE_PREFIX, RENDER_FLAGS, trimTrailingNulls, unsupported} from './util'; +import {asLiteral, chainedInstruction, CONTEXT_NAME, getAttrsForDirectiveMatching, getInterpolationArgsLength, IMPLICIT_REFERENCE, invalid, NON_BINDABLE_ATTR, REFERENCE_PREFIX, RENDER_FLAGS, RESTORED_VIEW_CONTEXT_NAME, trimTrailingNulls, unsupported} from './util'; @@ -83,8 +83,10 @@ export function prepareEventListenerParameters( eventAst.handlerSpan, implicitReceiverAccesses, EVENT_BINDING_SCOPE_GLOBALS); const statements = []; if (scope) { - statements.push(...scope.restoreViewStatement()); + // `variableDeclarations` needs to run first, because + // `restoreViewStatement` depends on the result. statements.push(...scope.variableDeclarations()); + statements.unshift(...scope.restoreViewStatement()); } statements.push(...bindingExpr.render3Stmts); @@ -358,8 +360,17 @@ export class TemplateDefinitionBuilder implements t.Visitor, LocalResolver (scope: BindingScope, relativeLevel: number) => { let rhs: o.Expression; if (scope.bindingLevel === retrievalLevel) { - // e.g. ctx - rhs = o.variable(CONTEXT_NAME); + if (scope.isListenerScope() && scope.hasRestoreViewVariable()) { + // e.g. restoredCtx. + // We have to get the context from a view reference, if one is available, because + // the context that was passed in during creation may not be correct anymore. + // For more information see: https://github.com/angular/angular/pull/40360. + rhs = o.variable(RESTORED_VIEW_CONTEXT_NAME); + scope.notifyRestoredViewContextUse(); + } else { + // e.g. ctx + rhs = o.variable(CONTEXT_NAME); + } } else { const sharedCtxVar = scope.getSharedContextName(retrievalLevel); // e.g. ctx_r0 OR x(2); @@ -1658,6 +1669,7 @@ export class BindingScope implements LocalResolver { private map = new Map(); private referenceNameIndex = 0; private restoreViewVariable: o.ReadVarExpr|null = null; + private usesRestoredViewContext = false; static createRootScope(): BindingScope { return new BindingScope(); } @@ -1833,17 +1845,23 @@ export class BindingScope implements LocalResolver { } restoreViewStatement(): o.Statement[] { - // restoreView($state$); - return this.restoreViewVariable ? - [instruction(null, R3.restoreView, [this.restoreViewVariable]).toStmt()] : - []; + const statements: o.Statement[] = []; + if (this.restoreViewVariable) { + const restoreCall = instruction(null, R3.restoreView, [this.restoreViewVariable]); + // Either `const restoredCtx = restoreView($state$);` or `restoreView($state$);` + // depending on whether it is being used. + statements.push( + this.usesRestoredViewContext ? + o.variable(RESTORED_VIEW_CONTEXT_NAME).set(restoreCall).toConstDecl() : + restoreCall.toStmt()); + } + return statements; } viewSnapshotStatements(): o.Statement[] { // const $state$ = getCurrentView(); - const getCurrentViewInstruction = instruction(null, R3.getCurrentView, []); return this.restoreViewVariable ? - [this.restoreViewVariable.set(getCurrentViewInstruction).toConstDecl()] : + [this.restoreViewVariable.set(instruction(null, R3.getCurrentView, [])).toConstDecl()] : []; } @@ -1873,6 +1891,14 @@ export class BindingScope implements LocalResolver { const ref = `${REFERENCE_PREFIX}${current.referenceNameIndex++}`; return ref; } + + hasRestoreViewVariable(): boolean { + return !!this.restoreViewVariable; + } + + notifyRestoredViewContextUse(): void { + this.usesRestoredViewContext = true; + } } /** diff --git a/packages/compiler/src/render3/view/util.ts b/packages/compiler/src/render3/view/util.ts index 8f37e2d7f5..e237933a98 100644 --- a/packages/compiler/src/render3/view/util.ts +++ b/packages/compiler/src/render3/view/util.ts @@ -45,6 +45,9 @@ export const IMPLICIT_REFERENCE = '$implicit'; /** Non bindable attribute name **/ export const NON_BINDABLE_ATTR = 'ngNonBindable'; +/** Name for the variable keeping track of the context returned by `ɵɵrestoreView`. */ +export const RESTORED_VIEW_CONTEXT_NAME = 'restoredCtx'; + /** * Creates an allocator for a temporary variable. * diff --git a/packages/core/src/render3/state.ts b/packages/core/src/render3/state.ts index 0217e2f60d..5d1e02f106 100644 --- a/packages/core/src/render3/state.ts +++ b/packages/core/src/render3/state.ts @@ -284,11 +284,13 @@ export function getTView(): TView { * walking the declaration view tree in listeners to get vars from parent views. * * @param viewToRestore The OpaqueViewState instance to restore. + * @returns Context of the restored OpaqueViewState instance. * * @codeGenApi */ -export function ɵɵrestoreView(viewToRestore: OpaqueViewState) { +export function ɵɵrestoreView(viewToRestore: OpaqueViewState): T { instructionState.lFrame.contextLView = viewToRestore as any as LView; + return (viewToRestore as any as LView)[CONTEXT] as T; }