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

add console-error to the Logger class #7125

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

pmario
Copy link
Member

@pmario pmario commented Dec 15, 2022

This PR allows the Logger class to use console.error() ... since console.log() doesn't add any color to messages in the browser dev-console.

This is needed, if a core function throws an error and there is a try/catch function. If possible the catch() function should avoid data corruption.


There is a $tw.utils.error() function which would write a message to the browser console. BUT it also creates the RSOD info. .. So it's not usable.

$tw.utils.log() is not usable for my usecase, since it's not visible enough.


An alternative would be to create global $tw.utils.errorToConsole() utility function. .. BUT I think the implementation in the PR is more versatile.


If the PR is merged I'll add some docs to the tiddlywiki.com/dev edition. ... Because messing around with the naming inconsistencies did cost me 3 hours.

So investing 2 more to create some docs about what I learned IMO would be OK

@vercel
Copy link

vercel bot commented Dec 15, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
tiddlywiki5 ✅ Ready (Inspect) Visit Preview Jun 21, 2024 7:07am

@pmario
Copy link
Member Author

pmario commented Jan 15, 2023

bump

@Jermolene
Copy link
Member

Hi @pmario I notice that the new error() method does not save output to the buffer as we do in the log() method. Is there a reason for this? It seems that with use of a factory function the two methods could share most of their implementation.

But I'm concerned at a deeper level at the proliferation of similar but different debugging methods.

What is the underlying problem you want to solve? If it is to add colour to log messages I'd rather we approached that by trying to make the browser and server more compatible in that regard.

This is needed, if a core function throws an error and there is a try/catch function. If possible the catch() function should avoid data corruption.

I don't quite understand. Are you suggesting that there are places in the core that we need to add error trapping and use this proposed new method?

@pmario
Copy link
Member Author

pmario commented Jan 18, 2023

I notice that the new error() method does not save output to the buffer as we do in the log() method. Is there a reason for this

I did search the whole code-base and logger.getBuffer() is only used once in the syncer.

There is no info about the "args" in the Logger.prototype.log = function(/* args */) { .. so I don't really know, what the function exactly should do and which parameters can be assigned in which order ...

Since the existing logger is unable to create a console.error() it was the only thing the new function should do. So I implemented it that way.

I could have added a new parameter to the .log(/*args*/), but I don't know how, without causing side effects.

@pmario
Copy link
Member Author

pmario commented Jan 18, 2023

This is needed, if a core function throws an error and there is a try/catch function. If possible the catch() function should avoid data corruption.

I don't quite understand. Are you suggesting that there are places in the core that we need to add error trapping and use this proposed new method?

No I wanted to have a log function for plugins to call if an error message should be logged to the console in the same way plugins can use the .log() method. .. Without he RSOD

@pmario pmario marked this pull request as draft August 1, 2023 08:45
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.

2 participants