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

Error handling: Editor debug mode #1303

Closed
pjasiun opened this issue Oct 15, 2018 · 5 comments
Closed

Error handling: Editor debug mode #1303

pjasiun opened this issue Oct 15, 2018 · 5 comments
Labels
type:improvement This issue reports a possible enhancement of an existing feature.
Milestone

Comments

@pjasiun
Copy link

pjasiun commented Oct 15, 2018

There should be a global DEBUG flag to switch the editor to the debug mode. Editor in this mode should log more errors, in all cases we know that might be incorrect, but we do not want to log errors in the production environment (cause the case might be correct as well). We have more and more such cases so it should be useful.

All editor manual tests should run in this mode by default.

Related tickets: https://github.com/ckeditor/ckeditor5-engine/issues/824, ckeditor/ckeditor5-utils#242 (comment)

@pjasiun pjasiun added type:improvement This issue reports a possible enhancement of an existing feature. status:confirmed labels Oct 15, 2018
@Reinmar
Copy link
Member

Reinmar commented Oct 15, 2018

There should be a global DEBUG flag

Or even CKEDITOR_DEBUG ;)

Editor in this mode should log more errors, in all cases we know that might be incorrect, but we do not want to log errors in the production environment (cause the case might be correct as well).

Just... not too many. Please remember about code size.

Additionally, this flag needs to be documented somewhere. The only place I can think of is https://ckeditor.com/docs/ckeditor5/latest/framework/guides/support/error-codes.html

Also, how do you plan to read this variable from the global scope? Please remember about problems that we had in Electron with the global object (you'd need to dig what problems that were).

@ma2ciek
Copy link
Contributor

ma2ciek commented Oct 16, 2018

In the electron we have an access via both global and window to the window object AFAIR (#879 (comment)). The problem was with the check if an object is the window object.

@Reinmar Reinmar added this to the next milestone Apr 16, 2019
@mlewand
Copy link
Contributor

mlewand commented Apr 16, 2019

Instead of a single flag, we could even go with a verbosity level, like notice, warning, errors etc.

It would be nice to integrate it with CKE5 inspector, so that it contains tab with logs. It might contain a badge with number of each logs, so that it somehow hints that some warnings / errors were reported.

@Reinmar
Copy link
Member

Reinmar commented Apr 25, 2019

Instead of a single flag, we could even go with a verbosity level, like notice, warning, errors etc.

We have errors on the console right now, because developers need to be notified about them anyway because they usually mean an app crash. I remember some strange situations in which people wanted to disabled those, but that's not really doable (since we throw errors).

Then there are warnings, but to me all (or most) of the warnings that we have right now are really errors. So they fall into the same category as errors – stuff you don't want/can disable.

So, what we're really looking for here are "non-crash-related" things like "it's wrong but the app should still work". One of those things currently are infinite loops in the selection renderer. Those type of warnings can be optional. But the app should not flood you with them because then it means we've got too many smelly situations and we've got a bigger issue somewhere.

Finally, there can be some useful information logged. Like: rendering, converting, selection change, etc. Stuff that might not be really visible at first and sometimes you don't even realise that this thing is happening (and perhaps it shouldn't). Also, the order is always interesting.

HOWEVER, those things happen so often, that if we log all that (if someone enabled "info level debugging") then it's completely useless. Therefore, I wonder if we could instead have a filter like "show me this and this and this". This sounds like a great job for the inspector. You'd have a series of checkboxes – for stuff that you want to get logs.

I'd see this implemented in the inspector based on events that we fire (if we miss some, we should add them). I even proposed this in the past (look for "event viewer" in #1205). But I see this as a completely separate from "Editor debug mode".

tl;dr;

  • I don't think we need verbosity levels. A flag turning warnings on/off will be fine. They should be printed to the native console. We should also review the existing warnings, because I feel that some of them are really errors.
  • We need an event viewer in the inspector.

@pjasiun pjasiun changed the title Editor debug mode Error handling: Editor debug mode Jun 7, 2019
@ma2ciek
Copy link
Contributor

ma2ciek commented Aug 23, 2019

It's closed by #383.

The only thing missing is treating console.warns and console.errors on CI as real errors.

@ma2ciek ma2ciek closed this as completed Aug 23, 2019
@ma2ciek ma2ciek modified the milestones: nice-to-have, iteration 26 Aug 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:improvement This issue reports a possible enhancement of an existing feature.
Projects
None yet
Development

No branches or pull requests

4 participants