Skip to content

feat(core): introduce Logger Service - #17399

Closed
Toxicable wants to merge 1 commit into
angular:masterfrom
Toxicable:logger-service
Closed

feat(core): introduce Logger Service#17399
Toxicable wants to merge 1 commit into
angular:masterfrom
Toxicable:logger-service

Conversation

@Toxicable

@Toxicable Toxicable commented Jun 10, 2017 •
edited
Loading

Copy link
Copy Markdown

Please check if the PR fulfills these requirements

  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

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

- [x] Feature

What is the current behavior? (You can also link to an open issue here)
There is no logging service like AngularJS has as described here #5458

What is the new behavior?
Adds in an AbstractLogger with a default implementation of ConsoleLogger

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

- [x] No

Other information:
Closes #5458
Duplicate of #14308 but that appears to have gone stale
Will be able to fix several TODO's in a follow up PR such as : https://github.com/angular/angular/blob/master/packages/common/src/directives/ng_for_of.ts#L105

@Toxicable Toxicable changed the title feat(core): introduce Logger Service [WIP] feat(core): introduce Logger Service Jun 10, 2017
@Toxicable
Toxicable force-pushed the logger-service branch 9 times, most recently from 9583a76 to fdec87d Compare June 10, 2017 23:56
@Toxicable Toxicable changed the title [WIP] feat(core): introduce Logger Service feat(core): introduce Logger Service Jun 11, 2017
@RainingNight

RainingNight commented Jul 7, 2017

Copy link
Copy Markdown

very good

@IgorMinar IgorMinar added area: core Issues related to the framework runtime state: blocked labels Jul 11, 2017
@IgorMinar
IgorMinar self-requested a review July 11, 2017 22:47
IgorMinar
IgorMinar suggested changes Jul 11, 2017
View reviewed changes

@IgorMinar IgorMinar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

reposting my review summary from #14308:

My primary issue with this PR is that logging typically becomes very expensive (in terms of payload-size) very quickly. Until we have a design that can safely strip non-critical logs and minify the critical ones I think everyone is better off rolling their own solution. The api above would likely need to be changed to achieve these properties, and doing so after these apis are heavily used would be very disruptive and painful for everyone.

The cross-cutting concern of logging and necessity to provide a solution that works well for 3rd party components makes this a much harder problem than what it looks like on the surface and we are better off deferring tackling this problem rather than providing a solution that would send everyone down the wrong path.

@Toxicable

Toxicable commented Jul 12, 2017 •
edited
Loading

Copy link
Copy Markdown
Author

@IgorMinar That sounds reasonable to me, thanks for the heads up.
Recall that Services are not tree shakable so as per your concerns a LoggerService is not acceptable.
I will investigate other methods for how we could implement this in a shakable way, my first thought would be to export it as a basic class rather than an Injectable class, then allow the user to inject it if they want to use it, otherwise it would be shaked away. The issue with this kinda of API is that it would require the user to write a bit more boilerplate and wouldn't be very nice to use compared to other API's.

However, if it would be possible to make a change in how the Injector worked to make Services shakable that would solve everything! But I assume that's a rather difficult problem to solve.

@chuckjaz chuckjaz added feature Label used to distinguish feature request from other issues state: Needs Design labels Jul 12, 2017
@devoto13 devoto13 mentioned this pull request Jul 13, 2017
Closed
@Toxicable Toxicable mentioned this pull request Aug 3, 2017
Closed
@Toxicable Toxicable closed this Sep 28, 2017
@angular-automatic-lock-bot

angular-automatic-lock-bot Bot commented Sep 12, 2019

Copy link
Copy Markdown

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 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer
@IgorMinar IgorMinar IgorMinar requested changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area: core Issues related to the framework runtime cla: yes feature Label used to distinguish feature request from other issues state: blocked state: Needs Design

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add $log like functionality to Angular 2

5 participants

@Toxicable @RainingNight @IgorMinar @chuckjaz @googlebot