fix: allow context of EmbeddedViewRef to be changed and avoid mutating context in NgTemplateOutlet by crisbeto · Pull Request #40360 · angular/angular

crisbeto · 2021-01-08T12:44:58Z

Currently NgTemplateOutlet recreates its view if its template is swapped out or a context object with a different shape is passed in. If an object with the same shape is passed in, we preserve the old view and we mutate the previous object. This mutation of the original object can be undesirable if two objects with the same shape are swapped between two different template outlets.

The current behavior is a result of a limitation in core where the context of an embedded view is read-only.

These changes are split up into two commits which aim to resolve the issue:

The first commit makes EmbeddedViewRef.context writeable so that the object doesn't have to be mutated anymore and there didn't seem to be a good reason why it was read-only to begin with.
The second commit resolves the context mutation issue by swapping out the context, rather than mutating it, when a new context object comes into NgTemplateOutlet.

Fixes #24515.

atscott

reviewed-for: fw-core
reviewed-for: public-api
reviewed-for: size-tracking

atscott · 2021-01-11T18:07:25Z

presubmit

atscott · 2021-01-13T00:11:57Z

FYI - it looks like there might be a real failure in g3 due to this change. I haven't gotten a chance to investigate yet, but the bit of code in the template that seems to be broken is

        <ng-template [ngTemplateOutlet]="createFormTemplate"
                     [ngTemplateOutletContext]="{$implicit: (sourceBackup$ | async)}">
        </ng-template>
        <ng-template #createFormTemplate let-sourceBackup>
          <form [formGroup]="createForm"
                (ngSubmit)="create(sourceBackup)">
           ...
        </ng-template>

and then the failure is Cannot read property 'name' of undefined from the create method trying to read sourceBackup, which must be undefined. I won't get to investigating this until tomorrow, but maybe that's enough info for you to get started before then.

packages/common/src/directives/ng_template_outlet.ts

zarend · 2021-02-18T17:26:07Z

presubmit

AndrewKushnir

Reviewed-for: public-api

petebacondarwin · 2021-02-20T13:01:50Z

packages/core/src/linker/view_ref.ts

   * The context for this view, inherited from the anchor element.
   */
-  abstract get context(): C;
+  abstract context: C;


Strictly this is a breaking change in the case that this class EmbeddedViewRef was extended. See playground.

Do we consider this as something that's meant to be extended, considering that the framework itself constructs it? I believe we've had cases in the past where we've said that constructor changes (also something that would be breaking for extending consumers) aren't breaking.

I'm not sure but we could avoid the breaking change by just adding a setter, rather than making it a property.

I wanted to avoid getters and setters, because they generate more code in ES5.

And actually adding an abstract setter would also be a breaking change!

Unless a class is specifically noted that it’s meant to be extended, we don’t consider breaking extenders a breaking change. See #32057 (comment)

So if we are cool that no one outside the framework is going to extend this then I am cool with it

petebacondarwin

Reviewed-for: public-api

petebacondarwin · 2021-02-20T14:50:19Z

packages/core/src/linker/view_ref.ts

   * The context for this view, inherited from the anchor element.
   */
-  abstract get context(): C;
+  abstract context: C;


So if we are cool that no one outside the framework is going to extend this then I am cool with it

jessicajaniuk

reviewed-for: size-tracking

Currently `NgTemplateOutlet` recreates its view if its template is swapped out or a context object with a different shape is passed in. If an object with the same shape is passed in, we preserve the old view and we mutate the previous object. This mutation of the original object can be undesirable if two objects with the same shape are swapped between two different template outlets. The current behavior is a result of a limitation in `core` where the `context` of an embedded view is read-only, however a previous commit made it writeable. These changes resolve the context mutation issue and clean up a bunch of unnecessary logic from `NgTemplateOutlet` by taking advantage of the earlier change. Fixes #24515. PR Close #40360

angular-automatic-lock-bot · 2021-03-26T16:08:29Z

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

google-cla bot added the cla: yes label Jan 8, 2021

crisbeto force-pushed the 24515/embedded-view-ref-context branch from 8f83124 to 38174ea Compare January 8, 2021 13:11

crisbeto marked this pull request as ready for review January 8, 2021 13:31

pullapprove bot requested a review from atscott January 8, 2021 13:31

crisbeto added action: review The PR is still awaiting reviews from at least one requested reviewer area: common Issues related to APIs in the @angular/common package area: core Issues related to the framework runtime target: patch This PR is targeted for the next patch release labels Jan 8, 2021

ngbot bot modified the milestone: Backlog Jan 8, 2021

crisbeto added the type: bug/fix label Jan 8, 2021

crisbeto mentioned this pull request Jan 11, 2021

User ngTemplateOutletContext been modified unexpectedly #40385

Closed

atscott approved these changes Jan 11, 2021

View reviewed changes

pullapprove bot requested a review from atscott January 11, 2021 18:04

crisbeto added action: merge The PR is ready for merge by the caretaker 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 Jan 11, 2021

atscott removed the action: merge The PR is ready for merge by the caretaker label Jan 13, 2021

atscott requested changes Jan 13, 2021

View reviewed changes

packages/common/src/directives/ng_template_outlet.ts Outdated Show resolved Hide resolved

crisbeto added target: major This PR is targeted for the next major release and removed target: patch This PR is targeted for the next patch release labels Jan 14, 2021

AndrewKushnir modified the milestones: Backlog, v12-candidates Jan 14, 2021

AndrewKushnir removed the action: presubmit The PR is in need of a google3 presubmit label Jan 14, 2021

JoostK mentioned this pull request Jan 20, 2021

[NgTemplateOutlet] _updateExistingContext causes side effects #40493

Closed

crisbeto force-pushed the 24515/embedded-view-ref-context branch from 38174ea to e6e6d67 Compare January 23, 2021 09:53

crisbeto changed the title ~~fix: allow context of EmbeddedViewRef to be changed and avoid mutation context in NgTemplateOutlet~~ fix: allow context of EmbeddedViewRef to be changed and avoid mutating context in NgTemplateOutlet Jan 23, 2021

crisbeto force-pushed the 24515/embedded-view-ref-context branch from e6e6d67 to f7ff356 Compare January 23, 2021 10:13

crisbeto added action: merge The PR is ready for merge by the caretaker action: presubmit The PR is in need of a google3 presubmit labels Feb 18, 2021

This comment has been minimized.

Sign in to view

atscott removed the action: presubmit The PR is in need of a google3 presubmit label Feb 19, 2021

atscott approved these changes Feb 19, 2021

View reviewed changes

pullapprove bot requested review from AndrewKushnir, IgorMinar, alxhub and petebacondarwin February 19, 2021 17:28

AndrewKushnir approved these changes Feb 19, 2021

View reviewed changes

pullapprove bot requested a review from jelbourn February 19, 2021 20:23

petebacondarwin reviewed Feb 20, 2021

View reviewed changes

pullapprove bot requested a review from jessicajaniuk February 20, 2021 13:02

petebacondarwin approved these changes Feb 20, 2021

View reviewed changes

jessicajaniuk approved these changes Feb 22, 2021

View reviewed changes

atscott removed request for IgorMinar, alxhub and jelbourn February 22, 2021 20:44

atscott closed this in a3e1719 Feb 23, 2021

angular-automatic-lock-bot bot locked and limited conversation to collaborators Mar 26, 2021

Conversation

crisbeto commented Jan 8, 2021

Uh oh!

atscott left a comment

Choose a reason for hiding this comment

Uh oh!

atscott commented Jan 11, 2021

Uh oh!

atscott commented Jan 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

zarend commented Feb 18, 2021

Uh oh!

AndrewKushnir left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petebacondarwin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jessicajaniuk left a comment

Choose a reason for hiding this comment

Uh oh!

angular-automatic-lock-bot bot commented Mar 26, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

atscott commented Jan 13, 2021 •

edited

Loading