fix: allow context of EmbeddedViewRef to be changed and avoid mutating context in NgTemplateOutlet

[crisbeto]

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:

1. 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.
2. 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]

presubmit

[atscott]

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.

[zarend]

presubmit

[AndrewKushnir]

Reviewed-for: public-api

[petebacondarwin]

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

[crisbeto]

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.

[petebacondarwin]

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

[crisbeto]

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

[petebacondarwin]

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

[atscott]

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)

[petebacondarwin]

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]

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

[angular-automatic-lock-bot]

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