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(dom_adapter): remove logError from logGroup #12925

Merged
merged 1 commit into from
Dec 9, 2016

Conversation

DzmitryShylovich
Copy link
Contributor

Fixes #12923

@DzmitryShylovich DzmitryShylovich changed the title fix(dom_adapter): properly bind execution context for console.log [WIP] fix(dom_adapter): properly bind execution context for console.log Nov 16, 2016
@DzmitryShylovich DzmitryShylovich changed the title [WIP] fix(dom_adapter): properly bind execution context for console.log fix(dom_adapter): remove logError from logGroup Nov 16, 2016
@@ -79,7 +79,7 @@ export class BrowserDomAdapter extends GenericBrowserDomAdapter {
// TODO(tbosch): move this into a separate environment class once we have it
logError(error: string): void {
if (window.console) {
(window.console.error || window.console.log)(error);
Copy link
Contributor

Choose a reason for hiding this comment

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

the behavior is not the same here (former is try error then log)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@@ -92,7 +92,6 @@ export class BrowserDomAdapter extends GenericBrowserDomAdapter {
logGroup(error: string): void {
if (window.console) {
window.console.group && window.console.group(error);
this.logError(error);
Copy link
Contributor

Choose a reason for hiding this comment

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

probably reasonable, have you check the call sites to see if we do not rely on this printing an error ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vicb vicb added action: merge The PR is ready for merge by the caretaker pr_state: LGTM labels Dec 9, 2016
@vicb vicb merged commit 8a8c532 into angular:master Dec 9, 2016
@DzmitryShylovich DzmitryShylovich deleted the gh/12923 branch December 9, 2016 23:40
@angular-automatic-lock-bot
Copy link

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 The PR is ready for merge by the caretaker cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Router throws Illegal Invocation error when enableTracing is true
3 participants