From 74ccf76e1b36ce804c08ec2abcd3a2ca9c56e4c6 Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Sat, 23 Sep 2023 10:40:02 +0000 Subject: [PATCH 1/4] test: add benchmark for ng template outlet context modifications Adds a benchmark for potential upcoming `NgTemplateOutlet` context logic updates. --- .../ng_template_outlet_context/BUILD.bazel | 14 ++++ .../ng2/BUILD.bazel | 39 +++++++++ .../ng_template_outlet_context/ng2/index.html | 16 ++++ .../ng_template_outlet_context/ng2/index.ts | 82 +++++++++++++++++++ .../ng_template_outlet_context.perf-spec.ts | 74 +++++++++++++++++ 5 files changed, 225 insertions(+) create mode 100644 modules/benchmarks/src/ng_template_outlet_context/BUILD.bazel create mode 100644 modules/benchmarks/src/ng_template_outlet_context/ng2/BUILD.bazel create mode 100644 modules/benchmarks/src/ng_template_outlet_context/ng2/index.html create mode 100644 modules/benchmarks/src/ng_template_outlet_context/ng2/index.ts create mode 100644 modules/benchmarks/src/ng_template_outlet_context/ng_template_outlet_context.perf-spec.ts diff --git a/modules/benchmarks/src/ng_template_outlet_context/BUILD.bazel b/modules/benchmarks/src/ng_template_outlet_context/BUILD.bazel new file mode 100644 index 000000000000..8930db9a4ecf --- /dev/null +++ b/modules/benchmarks/src/ng_template_outlet_context/BUILD.bazel @@ -0,0 +1,14 @@ +load("//tools:defaults.bzl", "ts_library") + +package(default_visibility = ["//visibility:public"]) + +ts_library( + name = "perf_lib", + testonly = True, + srcs = ["ng_template_outlet_context.perf-spec.ts"], + tsconfig = "//modules/benchmarks:tsconfig-e2e.json", + deps = [ + "@npm//@angular/build-tooling/bazel/benchmark/driver-utilities", + "@npm//protractor", + ], +) diff --git a/modules/benchmarks/src/ng_template_outlet_context/ng2/BUILD.bazel b/modules/benchmarks/src/ng_template_outlet_context/ng2/BUILD.bazel new file mode 100644 index 000000000000..72978f7a71be --- /dev/null +++ b/modules/benchmarks/src/ng_template_outlet_context/ng2/BUILD.bazel @@ -0,0 +1,39 @@ +load("//tools:defaults.bzl", "app_bundle", "http_server", "ng_module") +load("@npm//@angular/build-tooling/bazel/benchmark/component_benchmark:benchmark_test.bzl", "benchmark_test") + +package(default_visibility = ["//modules/benchmarks:__subpackages__"]) + +ng_module( + name = "ng2", + srcs = glob(["*.ts"]), + strict_templates = True, + tsconfig = "//modules/benchmarks:tsconfig-build.json", + deps = [ + "//modules/benchmarks/src:util_lib", + "//packages/core", + "//packages/platform-browser", + ], +) + +app_bundle( + name = "bundle", + entry_point = ":index.ts", + deps = [ + ":ng2", + ], +) + +http_server( + name = "prodserver", + srcs = ["index.html"], + deps = [ + ":bundle.debug.min.js", + "//packages/zone.js/bundles:zone.umd.js", + ], +) + +benchmark_test( + name = "perf", + server = ":prodserver", + deps = ["//modules/benchmarks/src/ng_template_outlet_context:perf_lib"], +) diff --git a/modules/benchmarks/src/ng_template_outlet_context/ng2/index.html b/modules/benchmarks/src/ng_template_outlet_context/ng2/index.html new file mode 100644 index 000000000000..206f7b0e568f --- /dev/null +++ b/modules/benchmarks/src/ng_template_outlet_context/ng2/index.html @@ -0,0 +1,16 @@ + + + + + + + + +

+ Loading... +

+ + + + + diff --git a/modules/benchmarks/src/ng_template_outlet_context/ng2/index.ts b/modules/benchmarks/src/ng_template_outlet_context/ng2/index.ts new file mode 100644 index 000000000000..a09f9c37788f --- /dev/null +++ b/modules/benchmarks/src/ng_template_outlet_context/ng2/index.ts @@ -0,0 +1,82 @@ +/** + * @license + * Copyright Google LLC All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ + +import {NgIf, NgTemplateOutlet} from '@angular/common'; +import {Component, enableProdMode, Input} from '@angular/core'; +import {bootstrapApplication} from '@angular/platform-browser'; + +@Component({ + selector: 'deep', + standalone: true, + imports: [NgIf], + template: ` Level: {{depth}}`, +}) +class Deep { + @Input({required: true}) depth: number; +} + +@Component({ + selector: 'app-component', + standalone: true, + imports: [NgTemplateOutlet, Deep], + template: ` + + + + + + +

Implicit: {{implicit}}

A: {{a}}

B: {{b}}

Deep: {{deep.next.text}}

New: {{new}}

+ + + + +

Outlet

+ +

+ `, +}) +class AppComponent { + context: + {$implicit: unknown, a: unknown, b: unknown, deep: {next: {text: unknown}}, new?: unknown} = { + $implicit: 'Default Implicit', + a: 'Default A', + b: 'Default B', + deep: {next: {text: 'Default deep text'}}, + }; + + swapOutFull() { + this.context = { + $implicit: 'New Implicit new Object', + a: 'New A new Object', + b: 'New B new Object', + deep: {next: {text: 'New Deep text new Object'}}, + }; + } + + modifyProperty() { + this.context.a = 'Modified a'; + } + + modifyDeepProperty() { + this.context.deep.next.text = 'Modified deep a'; + } + + addNewProperty() { + this.context.new = 'New property set'; + } +} + +enableProdMode(); +bootstrapApplication(AppComponent); diff --git a/modules/benchmarks/src/ng_template_outlet_context/ng_template_outlet_context.perf-spec.ts b/modules/benchmarks/src/ng_template_outlet_context/ng_template_outlet_context.perf-spec.ts new file mode 100644 index 000000000000..50c099a657ee --- /dev/null +++ b/modules/benchmarks/src/ng_template_outlet_context/ng_template_outlet_context.perf-spec.ts @@ -0,0 +1,74 @@ +/** + * @license + * Copyright Google LLC All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ + +import {runBenchmark, verifyNoBrowserErrors} from '@angular/build-tooling/bazel/benchmark/driver-utilities'; +import {$} from 'protractor'; + +interface Worker { + id: string; + prepare?(): void; + work(): void; +} + +const SwapFullContext = { + id: 'swapFullContext', + work: () => { + $('#swapOutFull').click(); + } +}; + +const ModifyContextProperty = { + id: 'modifyContextProperty', + work: () => { + $('#modifyProperty').click(); + } +}; + +const ModifyContextDeepProperty = { + id: 'modifyContextDeepProperty', + work: () => { + $('#modifyDeepProperty').click(); + } +}; + +const AddNewContextProperty = { + id: 'addNewContextProperty', + work: () => { + $('#addNewProperty').click(); + } +}; + +const scenarios = [ + SwapFullContext, + ModifyContextProperty, + ModifyContextDeepProperty, + AddNewContextProperty, +]; + +describe('ng_template_outlet_context benchmark spec', () => { + afterEach(verifyNoBrowserErrors); + + scenarios.forEach((worker) => { + describe(worker.id, () => { + it('should run for ng2', async () => { + await runBenchmarkScenario( + {url: '/', id: `ngTemplateOutletContext.ng2.${worker.id}`, worker: worker}); + }); + }); + }); + + function runBenchmarkScenario(config: {id: string, url: string, worker: Worker}) { + return runBenchmark({ + id: config.id, + url: config.url, + ignoreBrowserSynchronization: true, + prepare: config.worker.prepare, + work: config.worker.work + }); + } +}); From 20da81ccdbe5413434fae70c84bc4663a70643e2 Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Sat, 23 Sep 2023 10:40:42 +0000 Subject: [PATCH 2/4] build: fix IDE completion for benchmarks code We are including a file, for some reason, that is outside of the root dir. This breaks TypeScript/ or more specifically VSCode from picking up this tsconfig- breaking path mappings and hence auto completion. --- modules/benchmarks/tsconfig.json | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/modules/benchmarks/tsconfig.json b/modules/benchmarks/tsconfig.json index 020672cb0065..aa2d8c2d3616 100644 --- a/modules/benchmarks/tsconfig.json +++ b/modules/benchmarks/tsconfig.json @@ -30,6 +30,5 @@ "no-floating-promises": true, "no-unused-expression": true, "no-unused-variable": true - }, - "include": ["../../node_modules/@angular/build-tooling/bazel/benchmark/driver-utilities/"] + } } From 084ffc15426896001d31e26fe20aa92decf31df2 Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Thu, 28 Sep 2023 13:12:28 +0000 Subject: [PATCH 3/4] refactor(core): deprecate allowing full context object to be replaced in `EmbeddedViewRef` This partially reverts commit a3e17190e7e7e0329ed3643299c24d5fd510b7d6 and deprecates behavior added. The context of an embedded view ref at some point was switched from a getter to an actual assignable property. This is something we revert as it introduces additional complexity for our generated code (in terms of closures capturing the `ctx`), creates technical limitations for Angular's internals and the usage pattern is rarely used (and can be addressed via simple assignments, `Object.assign` or the use of a proxy if replacing the full context object is still desirable) DEPRECATED: Swapping out the context object for `EmbeddedViewRef` is no longer supported. Support for this was introduced with v12.0.0, but this pattern is rarely used. There is no replacement, but you can use simple assignments in most cases, or `Object.assign , or alternatively still replace the full object by using a `Proxy` (see `NgTemplateOutlet` as an example). Also adds a warning if the deprecated --- packages/core/src/render3/view_ref.ts | 12 +++++++++++ .../core/test/acceptance/template_ref_spec.ts | 21 +++++++++++++++++++ 2 files changed, 33 insertions(+) diff --git a/packages/core/src/render3/view_ref.ts b/packages/core/src/render3/view_ref.ts index 2139b1c0f8f2..40080ea84b6d 100644 --- a/packages/core/src/render3/view_ref.ts +++ b/packages/core/src/render3/view_ref.ts @@ -63,7 +63,19 @@ export class ViewRef implements EmbeddedViewRef, InternalViewRef, ChangeDe return this._lView[CONTEXT] as unknown as T; } + /** + * @deprecated Replacing the full context object is not supported. Modify the context + * directly, or consider using a `Proxy` if you need to replace the full object. + * // TODO(devversion): Remove this. + */ set context(value: T) { + if (ngDevMode) { + // Note: We have a warning message here because the `@deprecated` JSDoc will not be picked + // up for assignments on the setter. We want to let users know about the deprecated usage. + console.warn( + 'Angular: Replacing the `context` object of an `EmbeddedViewRef` is deprecated.'); + } + this._lView[CONTEXT] = value as unknown as {}; } diff --git a/packages/core/test/acceptance/template_ref_spec.ts b/packages/core/test/acceptance/template_ref_spec.ts index 58bc1fecb74b..c1c22702e6c6 100644 --- a/packages/core/test/acceptance/template_ref_spec.ts +++ b/packages/core/test/acceptance/template_ref_spec.ts @@ -317,5 +317,26 @@ describe('TemplateRef', () => { button.click(); expect(events).toEqual(['Frodo', 'Bilbo']); }); + + it('should warn if the context of an embedded view ref is replaced', () => { + TestBed.configureTestingModule({declarations: [App]}); + const fixture = TestBed.createComponent(App); + fixture.detectChanges(); + const viewRef = fixture.componentInstance.templateRef.createEmbeddedView({name: 'Frodo'}); + fixture.componentInstance.containerRef.insert(viewRef); + fixture.detectChanges(); + + expect(fixture.nativeElement.textContent).toBe('Frodo'); + spyOn(console, 'warn'); + + viewRef.context = {name: 'Bilbo'}; + fixture.detectChanges(); + + expect(console.warn).toHaveBeenCalledTimes(1); + expect(console.warn) + .toHaveBeenCalledWith(jasmine.stringContaining( + 'Replacing the `context` object of an `EmbeddedViewRef` is deprecated')); + expect(fixture.nativeElement.textContent).toBe('Bilbo'); + }); }); }); From f0b3730c4344caecd5149248ca4ca38bd85bbd3c Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Wed, 4 Oct 2023 11:08:45 +0000 Subject: [PATCH 4/4] refactor(common): update `NgTemplateOutlet` to no longer rely on context swapping The context of an embedded view ref at some point was switched from a getter to an actual assignable property. This is something we reverted with the previous commit as it introduces additional complexity for our generated code (in terms of closures capturing the `ctx`). This change impacted the template outlet code because we actively relied on swapping out the full context if the user changes it. Previousl, before we allowed to swap out the context (in v16), we mutated the initial view context if it didn't change structurally- and in other cases the view was re-created. We improved this performance aspect with the changes to allow for the context to be swapped out + actually also fixed a bug where the initial context object was mutated and the user could observe this change. This commit adjusts for context not being replacable- while still keeping the bugs fixed and preserving the performance wins of not having to destroy/re-create the view whenever the context changes. Benchmarks: https://hackmd.io/J0Ci_JzxQ0K1AA1omXhIQQ --- goldens/size-tracking/aio-payloads.json | 2 +- .../src/directives/ng_template_outlet.ts | 59 ++++++++++++++----- .../directives/ng_template_outlet_spec.ts | 24 ++++++++ 3 files changed, 68 insertions(+), 17 deletions(-) diff --git a/goldens/size-tracking/aio-payloads.json b/goldens/size-tracking/aio-payloads.json index d7e719e4a526..90aebc5a03bf 100755 --- a/goldens/size-tracking/aio-payloads.json +++ b/goldens/size-tracking/aio-payloads.json @@ -19,4 +19,4 @@ "dark-theme": 34598 } } -} +} \ No newline at end of file diff --git a/packages/common/src/directives/ng_template_outlet.ts b/packages/common/src/directives/ng_template_outlet.ts index 3d75130103be..46068372c8c9 100644 --- a/packages/common/src/directives/ng_template_outlet.ts +++ b/packages/common/src/directives/ng_template_outlet.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ -import {Directive, EmbeddedViewRef, Injector, Input, OnChanges, SimpleChanges, TemplateRef, ViewContainerRef} from '@angular/core'; +import {Directive, EmbeddedViewRef, Injector, Input, OnChanges, SimpleChange, SimpleChanges, TemplateRef, ViewContainerRef} from '@angular/core'; /** * @ngModule CommonModule @@ -57,30 +57,57 @@ export class NgTemplateOutlet implements OnChanges { constructor(private _viewContainerRef: ViewContainerRef) {} - /** @nodoc */ ngOnChanges(changes: SimpleChanges) { - if (changes['ngTemplateOutlet'] || changes['ngTemplateOutletInjector']) { + if (this._shouldRecreateView(changes)) { const viewContainerRef = this._viewContainerRef; if (this._viewRef) { viewContainerRef.remove(viewContainerRef.indexOf(this._viewRef)); } - if (this.ngTemplateOutlet) { - const { - ngTemplateOutlet: template, - ngTemplateOutletContext: context, - ngTemplateOutletInjector: injector, - } = this; - this._viewRef = - viewContainerRef.createEmbeddedView( - template, context, injector ? {injector} : undefined) as EmbeddedViewRef; - } else { + // If there is no outlet, clear the destroyed view ref. + if (!this.ngTemplateOutlet) { this._viewRef = null; + return; } - } else if ( - this._viewRef && changes['ngTemplateOutletContext'] && this.ngTemplateOutletContext) { - this._viewRef.context = this.ngTemplateOutletContext; + + // Create a context forward `Proxy` that will always bind to the user-specified context, + // without having to destroy and re-create views whenever the context changes. + const viewContext = this._createContextForwardProxy(); + this._viewRef = viewContainerRef.createEmbeddedView(this.ngTemplateOutlet, viewContext, { + injector: this.ngTemplateOutletInjector ?? undefined, + }); } } + + /** + * We need to re-create existing embedded view if either is true: + * - the outlet changed. + * - the injector changed. + */ + private _shouldRecreateView(changes: SimpleChanges): boolean { + return !!changes['ngTemplateOutlet'] || !!changes['ngTemplateOutletInjector']; + } + + /** + * For a given outlet instance, we create a proxy object that delegates + * to the user-specified context. This allows changing, or swapping out + * the context object completely without having to destroy/re-create the view. + */ + private _createContextForwardProxy(): C { + return new Proxy({}, { + set: (_target, prop, newValue) => { + if (!this.ngTemplateOutletContext) { + return false; + } + return Reflect.set(this.ngTemplateOutletContext, prop, newValue); + }, + get: (_target, prop, receiver) => { + if (!this.ngTemplateOutletContext) { + return undefined; + } + return Reflect.get(this.ngTemplateOutletContext, prop, receiver); + }, + }); + } } diff --git a/packages/common/test/directives/ng_template_outlet_spec.ts b/packages/common/test/directives/ng_template_outlet_spec.ts index eb3fdd2a0b66..f6ffc47d48ff 100644 --- a/packages/common/test/directives/ng_template_outlet_spec.ts +++ b/packages/common/test/directives/ng_template_outlet_spec.ts @@ -318,6 +318,30 @@ describe('NgTemplateOutlet', () => { expect(fixture.nativeElement.textContent).toBe('Hello World'); }); + + it('should properly bind context if context is unset initially', () => { + @Component({ + imports: [NgTemplateOutlet], + template: ` + Name:{{name}} + + `, + standalone: true, + }) + class TestComponent { + ctx: {$implicit: string}|undefined = undefined; + } + + const fixture = TestBed.createComponent(TestComponent); + fixture.detectChanges(); + + expect(fixture.nativeElement.textContent).toBe('Name:'); + + fixture.componentInstance.ctx = {$implicit: 'Angular'}; + fixture.detectChanges(); + + expect(fixture.nativeElement.textContent).toBe('Name:Angular'); + }); }); const templateToken = new InjectionToken('templateToken');