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

Is there a way to hook into PTT warnings #204

Open
rizen opened this issue Feb 22, 2022 · 8 comments
Open

Is there a way to hook into PTT warnings #204

rizen opened this issue Feb 22, 2022 · 8 comments
Assignees
Labels
enhancement New feature or request

Comments

@rizen
Copy link
Collaborator

rizen commented Feb 22, 2022

PTT puts warnings (like unclosed tags) into the console. Is there a way to hook into those so I could display them to the user?

@mimshwright
Copy link
Owner

There are two possible solutions:

  • Rather than logging warnings, the library can throw an error. The errors can be caught by your app and handled. (by default we don't do this because it can end up making life harder for the users of the library and frankly there are a lot of people who don't know how to handle errors)

  • you can override (hack) console.warn with a custom function that can receive the message and decide what to do with it.

Probably the best solution is to provide an optional setting for whether to throw errors or show console messages (or neither).

@mimshwright
Copy link
Owner

Maybe a third solution is to save the error state in code somewhere. Like taggedText.errors

@rizen
Copy link
Collaborator Author

rizen commented Feb 22, 2022

Maybe another solution is to pass a warnings handler function into options and the default warnings handler is to console.warn() like you do now, but if I've passed one use that instead?

@mimshwright
Copy link
Owner

mimshwright commented Feb 22, 2022

You could also have methods like validateTags() which could return a result for each type of issue. I guess there are many more than 2 possible solutions!

These are all pretty easy to add. Here's what I'd propose:

  1. Your idea: an option for adding a callback/handler for any errors that receives an object with target, type, and description.
  2. And an option for how to do warnings, either "off", "warn", "error" with default to "warn"

@rizen
Copy link
Collaborator Author

rizen commented Feb 22, 2022

Sounds fine

@mimshwright mimshwright self-assigned this Feb 24, 2022
@mimshwright mimshwright added the enhancement New feature or request label Feb 24, 2022
mimshwright added a commit that referenced this issue Feb 25, 2022
Added errorHandler and supressConsole as options. errorHandler will be called whenever there's an
internal warning in the code such as an invalid color.

re #204
mimshwright added a commit that referenced this issue Feb 25, 2022
Created a dummy console to supress warnings in the console when running tests

re #204
@mimshwright
Copy link
Owner

@rizen
I've added this in version 3.3.0. You can provide a function that gets an object like { message:string, code:string, type:string }. There's also an option to supressConsole which disables console messages except for debugConsole messages.

Currently, there are only a handful of situations where you will receive one of these warnings, that is, any time when you would have gotten console.warn in the past. I am undecided on whether to expand to errors and wanted to get your opinion. It would require that I go through the codebase and reroute any code that normally would throw an error through this logger. Additionally, I'm not sure how best to handle all uncaught errors that might come up that not directly related to my code.

I hadn't given this topic a whole lot of thought in the past but there should probably be a rule like "if it only breaks the style, make it a warning not an error". For example, this code currently throws but probably should warn and use the default value "left":

throw new Error(
        `Unsupported alignment type ${align}! Use one of : "left", "right", "center", "justify"`
      );

@mimshwright
Copy link
Owner

mimshwright commented Feb 25, 2022

Crap. I forgot to add the target to the errorMessage object. I expect you'll want that so you can identify which text field has the error. I'll add that.

Update: added in v3.3.1

mimshwright added a commit that referenced this issue Feb 25, 2022
Added a target property for error messages that references the thing that sent the error message.

re #204
@mimshwright
Copy link
Owner

Note to self, fix warnings on percentage font size

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants