Deprecate replacing full context object of `EmbeddedViewRef` #51887

devversion · 2023-09-25T07:17:33Z

See individual commits

packages/common/src/directives/ng_template_outlet.ts

crisbeto

LGTM

devversion · 2023-09-29T10:35:21Z

TGP

packages/common/src/directives/ng_template_outlet.ts

pkozlowski-opensource

LGTM. I think I was confused about the sequence of changes between the 4 commits. The end version looks good, but the 3rd commit was a bit surprising.

pkozlowski-opensource

LGTM

Reviewed-for: public-api
Reviewed-for: size-tracking
Reviewed-for: fw-core
Reviewed-for: fw-common

Adds a benchmark for potential upcoming `NgTemplateOutlet` context logic updates.

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.

… in `EmbeddedViewRef` This partially reverts commit a3e1719 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

…ext 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

devversion · 2023-10-04T12:02:36Z

@pkozlowski-opensource fixed the commit confusion.

Caretaker note: Please ignore the saucelabs check. These are unrelated failures.

alxhub · 2023-10-04T15:14:37Z

This PR was merged into the repository by commit 7426948.

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. PR Close #51887

… in `EmbeddedViewRef` (#51887) This partially reverts commit a3e1719 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 PR Close #51887

…ext swapping (#51887) 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 PR Close #51887

angular-automatic-lock-bot · 2023-11-04T00:08:46Z

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

…ular#51887) Adds a benchmark for potential upcoming `NgTemplateOutlet` context logic updates. PR Close angular#51887

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. PR Close angular#51887

… in `EmbeddedViewRef` (angular#51887) This partially reverts commit a3e1719 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 PR Close angular#51887

…ext swapping (angular#51887) 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 PR Close angular#51887

angular-robot bot added the area: build & ci Related the build and CI infrastructure of the project label Sep 25, 2023

ngbot bot added this to the Backlog milestone Sep 25, 2023

devversion force-pushed the context-mutate branch 2 times, most recently from 2452fe9 to afa3646 Compare September 25, 2023 10:06

devversion requested review from pkozlowski-opensource and crisbeto September 25, 2023 12:11

devversion force-pushed the context-mutate branch from afa3646 to 6457aae Compare September 25, 2023 12:37

angular-robot bot added the detected: breaking change PR contains a commit with a breaking change label Sep 25, 2023

devversion force-pushed the context-mutate branch from 6457aae to f19a60d Compare September 25, 2023 12:53

devversion changed the title ~~WIP: revert allowing context of EmbeddedViewRef to be assignable~~ revert allowing context of EmbeddedViewRef to be assignable Sep 25, 2023

devversion changed the title ~~revert allowing context of EmbeddedViewRef to be assignable~~ Revert making context of EmbeddedViewRef assignable Sep 25, 2023

devversion added area: common Issues related to APIs in the @angular/common package area: core Issues related to the framework runtime action: review The PR is still awaiting reviews from at least one requested reviewer target: major This PR is targeted for the next major release labels Sep 25, 2023

crisbeto reviewed Sep 26, 2023

View reviewed changes

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

devversion force-pushed the context-mutate branch from f19a60d to 7e9fff0 Compare September 27, 2023 15:26

devversion requested a review from crisbeto September 28, 2023 11:40

crisbeto approved these changes Sep 28, 2023

View reviewed changes

devversion force-pushed the context-mutate branch from 7e9fff0 to cd9b2c6 Compare September 28, 2023 13:22

angular-robot bot added detected: deprecation PR contains a commit with a deprecation and removed detected: breaking change PR contains a commit with a breaking change labels Sep 28, 2023

devversion force-pushed the context-mutate branch from cd9b2c6 to 56b990c Compare September 28, 2023 13:39

devversion requested a review from crisbeto September 28, 2023 13:40

devversion force-pushed the context-mutate branch 3 times, most recently from 5e19a46 to 6505d5b Compare September 29, 2023 10:35

devversion changed the title ~~Revert making context of EmbeddedViewRef assignable~~ Deprecate replacing full context object of EmbeddedViewRef Sep 29, 2023

devversion marked this pull request as ready for review September 29, 2023 10:36

crisbeto approved these changes Sep 29, 2023

View reviewed changes

pkozlowski-opensource reviewed Oct 4, 2023

View reviewed changes

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

pkozlowski-opensource reviewed Oct 4, 2023

View reviewed changes

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

pkozlowski-opensource approved these changes Oct 4, 2023

View reviewed changes

pullapprove bot requested review from alxhub and pkozlowski-opensource October 4, 2023 09:36

pkozlowski-opensource added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Oct 4, 2023

pkozlowski-opensource approved these changes Oct 4, 2023

View reviewed changes

pkozlowski-opensource removed the request for review from alxhub October 4, 2023 09:40

devversion added 4 commits October 4, 2023 11:08

test: add benchmark for ng template outlet context modifications

74ccf76

Adds a benchmark for potential upcoming `NgTemplateOutlet` context logic updates.

build: fix IDE completion for benchmarks code

20da81c

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.

devversion force-pushed the context-mutate branch from 6505d5b to f0b3730 Compare October 4, 2023 11:09

devversion added the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Oct 4, 2023

alxhub closed this in ffd3fca Oct 4, 2023

angular-automatic-lock-bot bot locked and limited conversation to collaborators Nov 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate replacing full context object of `EmbeddedViewRef` #51887

Deprecate replacing full context object of `EmbeddedViewRef` #51887

devversion commented Sep 25, 2023 •

edited

crisbeto left a comment

devversion commented Sep 29, 2023

pkozlowski-opensource left a comment

pkozlowski-opensource left a comment

devversion commented Oct 4, 2023

alxhub commented Oct 4, 2023

angular-automatic-lock-bot bot commented Nov 4, 2023

Deprecate replacing full context object of EmbeddedViewRef #51887

Deprecate replacing full context object of EmbeddedViewRef #51887

Conversation

devversion commented Sep 25, 2023 • edited

crisbeto left a comment

Choose a reason for hiding this comment

devversion commented Sep 29, 2023

pkozlowski-opensource left a comment

Choose a reason for hiding this comment

pkozlowski-opensource left a comment

Choose a reason for hiding this comment

devversion commented Oct 4, 2023

alxhub commented Oct 4, 2023

angular-automatic-lock-bot bot commented Nov 4, 2023

Deprecate replacing full context object of `EmbeddedViewRef` #51887

Deprecate replacing full context object of `EmbeddedViewRef` #51887

devversion commented Sep 25, 2023 •

edited