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

added ability to replace console #49

Closed
wants to merge 2 commits into from
Closed

added ability to replace console #49

wants to merge 2 commits into from

Conversation

VizuaaLOG
Copy link

This push implements two functions allowing loglevel to replace window.console but also restore it if required. I have done a manual test and both functions worked correctly.

This is to add the feature requested here: #42

self.restoreConsole = function() {
window.console = _console;
}

Copy link
Owner

Choose a reason for hiding this comment

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

Your indentation's not right here, can you fix that? (Probably tabs when it should be spaces)

@pimterry
Copy link
Owner

pimterry commented Aug 4, 2014

Thanks for picking this up! The build is currently failing here though, can you look at that?

On top of that, we're going to need some tests for this I think, primarily because I want to quickly run them in a range of other browsers and check that this doesn't break catastrophically anywhere.

In fact does this break setLevel functionality? What happens if you replace the console and then change the log level up and down? Just a bit worried because that tries to use the console object internally, and I think it might get quite confused if we've now replaced that.

@VizuaaLOG
Copy link
Author

@pimterry Thinking about it I think it might cause more problems than it might solve. Didn't think about other functions within the library that might use it.

@VizuaaLOG
Copy link
Author

@pimterry Ok, I have fixed the errors, and the build is now passing. I will have a look into the issues you mentioned about the plugin linking into the existing console api.

@pimterry
Copy link
Owner

Any further updates on this @VizuaaLOG?

@VizuaaLOG
Copy link
Author

@pimterry I haven't had the time to continue looking into it. So I haven't make any more changes.

@r4j4h
Copy link

r4j4h commented Nov 22, 2014

It's gross but maybe simply shimming setLevel to restore/run-setLevel/replace the console each call would suffice?

@pimterry
Copy link
Owner

Sounds plausible. Do you want to try putting together a pull request to see if that does work?

@r4j4h
Copy link

r4j4h commented May 1, 2015

It's been a while on this.. does anyone still feel this would still be useful? Let me know and I will put together a pull request if so.

@pimterry
Copy link
Owner

pimterry commented May 1, 2015

Yes, definitely useful, if you can find a way make it work. Feel free to have a go! We will need to have some tests in place too though to finish it, and we need to make sure the solution works nicely and reliably, as in my previous comment.

Don't want that to be too intimidating though. If you want to open a PR with any progress at all that'd be great, and we can go from there :-)

@r4j4h
Copy link

r4j4h commented May 9, 2015

Just reporting in that I have not had time to work on this but did a little bit of research I can share: Potential danger with changing console by calls to this binding function. I believe we can test by mocking two separate console objects and logging before and after switching between them, verifying that only the correct one is called and the incorrect one is not called each time. If that sounds satisfactory to everyone then I will hopefully get to it in the next few days. :)

@pimterry
Copy link
Owner

Been quiet for a long while now, so closing this. Feel free to bring this up to date and reopen it if you're still interested.

@pimterry pimterry closed this May 10, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants