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

Debug mode with custom Logger breaks #709

Closed
jonyeezs opened this issue Jan 28, 2020 · 5 comments
Closed

Debug mode with custom Logger breaks #709

jonyeezs opened this issue Jan 28, 2020 · 5 comments
Labels
bug For tagging faulty or unexpected behavior. pr-welcome We'd welcome a PR to solve the issue.

Comments

@jonyeezs
Copy link

Describe the bug
When having config showDebugInformation set as true and using a custom logger implementing OAuthLogger.
It throws an error where

console is undefined

To Reproduce
Steps to reproduce the behavior:

  1. Implement a OAuthLogger base on its contract.
  2. Have its injected service that is called internally
    export class CustomLogger {
       constructor() {
            this.thirdParty = {
                 execute: (a) => console.log('[3rd]', a);
           }
       }
    
      debug(...args) {
           this.thirdParty.execute(args);
      }
    }
  3. Add to that to the moudule
     { provide: OAuthLogger, useClass: CustomLogger }
  4. Have showDebugInformation enabled
 showDebugInformation: true,
  1. Run the app

Expected behavior
Should see the logs go through my custom thirdParty object.

Additional context
The problem i see is that it applies this to console, which i do not want.
I would expect it to just use the debug method and let the abstraction implement what it should do with the input.

@jonyeezs
Copy link
Author

I could turn the injector into a factory. But that's not really a good option for a nice UX DI experience as it is not following the contract API

@jeroenheijmans
Copy link
Collaborator

Hmm, I originally wrote the OAuthLogger type, with typings compatible with console because I was at the time just trying to replace console.log calls with an abstracted version.

Currently, the default is already provided through a factory, and you should be able to use a same approach with your own custom logger, non?

I suppose that could be changed so that the typings are more friendly to classes like your own with a ...args setup, though I do expect that changing the OAuthLogger typing would be a breaking change....

Does this provide you with enough info to fix something in your app, or do you still propose a specific change to the lib?

@jeroenheijmans jeroenheijmans added the investigation-needed Indication that the maintainer or involved community members may need to investigate more. label Jan 28, 2020
@jonyeezs
Copy link
Author

jonyeezs commented Jan 28, 2020 via email

@jeroenheijmans jeroenheijmans added bug For tagging faulty or unexpected behavior. pr-welcome We'd welcome a PR to solve the issue. and removed investigation-needed Indication that the maintainer or involved community members may need to investigate more. labels Jan 28, 2020
@jeroenheijmans
Copy link
Collaborator

Makes sense to me.

If someone cares to create a PR to update this that would be nice. (Fair warning though: PRs typically wait quite a bit of time before getting merged, usually only when a new Angular major version lands.)

@manfredsteyer
Copy link
Owner

Will work in version 9. lands soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug For tagging faulty or unexpected behavior. pr-welcome We'd welcome a PR to solve the issue.
Projects
None yet
Development

No branches or pull requests

3 participants