Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(enableDebugTools): create AngularTools at ng.tools #12003

Merged
merged 1 commit into from Nov 3, 2016

Conversation

danielcrisp
Copy link
Contributor

@danielcrisp danielcrisp commented Sep 30, 2016

Please check if the PR fulfills these requirements

What kind of change does this PR introduce? (check one with "x")

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

What is the current behavior? (You can also link to an open issue here)

ng.probe and ng.coreTokens are clobbered when you use enableDebugTools because the AngularTools class overwrites context.ng like this:

```javascript
context.ng = new AngularTools(ref);
```

Source: https://github.com/angular/angular/blob/master/modules/%40angular/platform-browser/src/browser/tools/tools.ts#L30

This means extensions like Augury no longer work because they use ng.probe.

More info: #12002

What is the new behavior?

AngularTools is now created at context.ng.tools so existing methods and properties on ng. are not affected.

This also allows disableDebugTools to remove the tools when required.

Unit tests and docs have been modified to incorporate this new location.

Does this PR introduce a breaking change? (check one with "x")

[ ] Yes
[x] No

If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...

Other information:

Fixes #12002

@@ -29,7 +29,7 @@ export class AngularTools {

/**
* Entry point for all Angular profiling-related debug tools. This object
* corresponds to the `ng.profiler` in the dev console.
* corresponds to the `ng.tools.profiler` in the dev console.
Copy link
Contributor

@vicb vicb Sep 30, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a way to fix this without breaking the API ?

Copy link
Contributor

@lacolaco lacolaco Oct 3, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't AngularTools has probe field?

Copy link
Contributor Author

@danielcrisp danielcrisp Oct 3, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vicb Yes, I could merge AngularTools into context.ng instead. Can I use Object.assign? I don't see it in use anywhere else in the source (apart from a test)

Copy link
Contributor Author

@danielcrisp danielcrisp Oct 3, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not keen on this suggestion because if additional methods are added to ng. someone would need to remember to keep this code up to date each time

When using `enableDebugTools` the tools are merged into `context.ng` to prevent `ng.probe` and `ng.coreTokens` from being clobbered.

Fixes angular#12002
@danielcrisp danielcrisp force-pushed the fix-ng-probe-enableDebugTools branch from 6faeab5 to 324cc6f Compare Oct 3, 2016
@danielcrisp
Copy link
Contributor Author

danielcrisp commented Oct 4, 2016

Updated using a merge approach instead to maintain the ng.profiler API

@vicb vicb added pr_state: LGTM action: merge and removed action: review labels Nov 2, 2016
@vikerman vikerman merged commit b2cf379 into angular:master Nov 3, 2016
3 checks passed
@danielcrisp danielcrisp deleted the fix-ng-probe-enableDebugTools branch Nov 3, 2016
@angular-automatic-lock-bot
Copy link

angular-automatic-lock-bot bot commented Sep 10, 2019

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge cla: yes comp: testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants