Introduce debugging APIs for structural inspection of an application #41525

mgechev · 2021-04-08T23:02:25Z

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

Feature

What is the new behavior?

Introduces new debugging utilities for structural inspection of an application. The PR expands the ng namespace and adds a new getDirectiveMetadata method that returns the metadata for a specified directive or component instance.

Additionally, the PR optimizes the getDirectives function by:

Returning an empty array if it's called with a text node
It invokes loadLContext with second argument false so it doesn't throw an error

Does this PR introduce a breaking change?

Yes, no longer throw an error (when ng.getDirectives is called) in case there are no directives on the element
No

packages/core/src/core_render3_private_export.ts

packages/core/src/debug/debug_node.ts

packages/core/src/render3/util/discovery_utils.ts

packages/core/src/render3/assert.ts

packages/core/src/render3/util/discovery_utils.ts

packages/core/src/render3/util/global_utils.ts

petebacondarwin · 2021-04-13T19:52:14Z

The aio-test failure is due to this line:

angular/aio/ngsw-config.json

Line 82 in 8c939dc

"!/api/**/DirectiveMetadata*",

This is because of this redirect in Firebase:

angular/aio/firebase.json

Line 86 in 820d95b

    
           {"type": 301, "source": "/**/DirectiveMetadata-*", "destination": "/api/core/Directive"},

I think we can fix the test by changing the ngsw-config line to have a hyphen:

    "!/api/**/DirectiveMetadata-*",

mary-poppins · 2021-04-13T21:11:45Z

You can preview a297877 at https://pr41525-a297877.ngbuilds.io/.

petebacondarwin · 2021-04-13T21:15:43Z

packages/core/src/render3/index.ts

@@ -176,12 +176,15 @@ export { ɵɵtemplateRefExtractor} from './view_engine_compatibility_prebound';
 // clang-format on

 export {
+  ComponentDebugMetadata as ComponentMetadata,


I don't think this (and the directive on below) is correct. We should not be aliasing these anymore.

Which is why they are appearing incorrectly in the API reference documentation: https://pr41525-a297877.ngbuilds.io/api/core/global#structures and are not clickable in the https://pr41525-a297877.ngbuilds.io/api/core/global/ngGetDirectiveMetadata doc

Yes, you're right. Code's refactoring introduced it. Removing them now.

petebacondarwin · 2021-04-13T21:17:27Z

packages/core/src/render3/index.ts

@@ -176,12 +176,15 @@ export { ɵɵtemplateRefExtractor} from './view_engine_compatibility_prebound';
 // clang-format on

 export {
+  ComponentDebugMetadata as ComponentMetadata,


Which is why they are appearing incorrectly in the API reference documentation: https://pr41525-a297877.ngbuilds.io/api/core/global#structures and are not clickable in the https://pr41525-a297877.ngbuilds.io/api/core/global/ngGetDirectiveMetadata doc

This commit introduces a global debugging method `ng.getDirectiveMetadata` which returns the metadata for a directive or component instance.

This commit introduces the following optimizations: 1. We return an empty array for text nodes in `getDirectives` because Angular does not support attaching logic to them. This optimization improves performance of `getDirectives` significantly because text nodes often result in expensive calls to `loadLContext` since we can't resolve it from a parent node. 1. `getDirectives` now calls `loadLContext` with second argument `false` so it doesn't throw an error. This brings another significant improvement because prevents the VM from deoptimizing calls. BREAKING CHANGE: Previously the `ng.getDirectives` function threw an error in case a given DOM node had no Angular context associated with it (for example if a function was called for a DOM element outside of an Angular app). This behavior was inconsistent with other debugging utilities under `ng` namespace, which handled this situation without raising an exception. Now calling the `ng.getDirectives` function for such DOM nodes would result in an empty array returned from that function.

mary-poppins · 2021-04-13T21:29:56Z

You can preview 9b60ae4 at https://pr41525-9b60ae4.ngbuilds.io/.

petebacondarwin

Reviewed-for: public-api

AndrewKushnir · 2021-04-13T22:08:08Z

Presubmit #3.

This commit introduces the following optimizations: 1. We return an empty array for text nodes in `getDirectives` because Angular does not support attaching logic to them. This optimization improves performance of `getDirectives` significantly because text nodes often result in expensive calls to `loadLContext` since we can't resolve it from a parent node. 1. `getDirectives` now calls `loadLContext` with second argument `false` so it doesn't throw an error. This brings another significant improvement because prevents the VM from deoptimizing calls. BREAKING CHANGE: Previously the `ng.getDirectives` function threw an error in case a given DOM node had no Angular context associated with it (for example if a function was called for a DOM element outside of an Angular app). This behavior was inconsistent with other debugging utilities under `ng` namespace, which handled this situation without raising an exception. Now calling the `ng.getDirectives` function for such DOM nodes would result in an empty array returned from that function. PR Close #41525

This commit refactors the code to replace `loadLContext` with `getLContext` calls. The only difference between these two functions is that the `loadLContext` supports throwing an error in case `LContext` can not be found. The investigation performed in angular#41525 revealed that throwing while retrieving `LContext` might have undesirable performance implications, so we should avoid that to make sure there are no accidental perf regressions in other parts of code that used `loadLContext`. Moreover, in most of the places the `loadLContext` was already called in a mode that prevented an error from being thrown, so this refactoring should have no effect on the actual behavior.

This commit refactors the code to replace `loadLContext` with `getLContext` calls. The only difference between these two functions is that the `loadLContext` supports throwing an error in case `LContext` can not be found. The investigation performed in #41525 revealed that throwing while retrieving `LContext` might have undesirable performance implications, so we should avoid that to make sure there are no accidental perf regressions in other parts of code that used `loadLContext`. Moreover, in most of the places the `loadLContext` was already called in a mode that prevented an error from being thrown, so this refactoring should have no effect on the actual behavior. PR Close #41606

angular-automatic-lock-bot · 2021-05-14T16:08:57Z

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 Apr 8, 2021

mgechev added the target: major This PR is targeted for the next major release label Apr 8, 2021

AndrewKushnir requested review from AndrewKushnir and mhevery April 8, 2021 23:07

zarend added the area: core Issues related to the framework runtime label Apr 8, 2021

ngbot bot added this to the Backlog milestone Apr 8, 2021

AndrewKushnir reviewed Apr 9, 2021

View reviewed changes

AndrewKushnir added action: review The PR is still awaiting reviews from at least one requested reviewer feature Issue that requests a new feature labels Apr 9, 2021

mgechev force-pushed the debug-node branch 3 times, most recently from 2567fc5 to 07cae06 Compare April 9, 2021 21:40

mgechev marked this pull request as ready for review April 9, 2021 21:41

mgechev force-pushed the debug-node branch 3 times, most recently from 0244d26 to 6505ad1 Compare April 9, 2021 21:50

JoostK reviewed Apr 9, 2021

View reviewed changes

packages/core/src/render3/util/discovery_utils.ts Outdated Show resolved Hide resolved

packages/core/src/render3/util/discovery_utils.ts Outdated Show resolved Hide resolved

JoostK reviewed Apr 9, 2021

View reviewed changes

packages/core/src/render3/util/discovery_utils.ts Outdated Show resolved Hide resolved

mgechev force-pushed the debug-node branch from 6505ad1 to aeb978c Compare April 9, 2021 22:05

AndrewKushnir reviewed Apr 9, 2021

View reviewed changes

mgechev force-pushed the debug-node branch 3 times, most recently from b3ab0e4 to 1c197ff Compare April 9, 2021 22:23

mhevery approved these changes Apr 9, 2021

View reviewed changes

mgechev force-pushed the debug-node branch from 1c197ff to fb8943e Compare April 9, 2021 23:13

AndrewKushnir added breaking changes action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Apr 9, 2021

mgechev force-pushed the debug-node branch from fb8943e to ab83db9 Compare April 9, 2021 23:34

pullapprove bot requested a review from AndrewKushnir April 9, 2021 23:34

mgechev force-pushed the debug-node branch from 90b4bf5 to a297877 Compare April 13, 2021 21:00

petebacondarwin reviewed Apr 13, 2021

View reviewed changes

petebacondarwin suggested changes Apr 13, 2021

View reviewed changes

mgechev added 2 commits April 13, 2021 14:19

feat(core): introduce getDirectiveMetadata global debugging utility

b619c08

This commit introduces a global debugging method `ng.getDirectiveMetadata` which returns the metadata for a directive or component instance.

mgechev force-pushed the debug-node branch from a297877 to 9b60ae4 Compare April 13, 2021 21:19

petebacondarwin approved these changes Apr 13, 2021

View reviewed changes

AndrewKushnir removed request for IgorMinar, atscott, jelbourn and alxhub April 13, 2021 21:58

AndrewKushnir 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: merge The PR is ready for merge by the caretaker labels Apr 13, 2021

AndrewKushnir added action: merge The PR is ready for merge by the caretaker and removed action: presubmit The PR is in need of a google3 presubmit labels Apr 13, 2021

zarend closed this in a07f303 Apr 13, 2021

mgechev deleted the debug-node branch April 13, 2021 23:12

AndrewKushnir mentioned this pull request Apr 14, 2021

refactor(core): replace loadLContext with getLContext calls #41606

Closed

3 tasks

angular-automatic-lock-bot bot locked and limited conversation to collaborators May 14, 2021

Introduce debugging APIs for structural inspection of an application #41525

Introduce debugging APIs for structural inspection of an application #41525

Uh oh!

Conversation

mgechev commented Apr 8, 2021 • edited by AndrewKushnir Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

PR Type

What is the new behavior?

Does this PR introduce a breaking change?

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

mary-poppins commented Apr 13, 2021

Uh oh!

petebacondarwin Apr 13, 2021

Choose a reason for hiding this comment

Uh oh!

petebacondarwin Apr 13, 2021

Choose a reason for hiding this comment

Uh oh!

mgechev Apr 13, 2021

Choose a reason for hiding this comment

Uh oh!

petebacondarwin Apr 13, 2021

Choose a reason for hiding this comment

Uh oh!

mary-poppins commented Apr 13, 2021

Uh oh!

petebacondarwin left a comment

Choose a reason for hiding this comment

Uh oh!

AndrewKushnir commented Apr 13, 2021

Uh oh!

angular-automatic-lock-bot bot commented May 14, 2021

Uh oh!

Uh oh!

mgechev commented Apr 8, 2021 •

edited by AndrewKushnir

Loading

petebacondarwin commented Apr 13, 2021 •

edited

Loading