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

Remove the "catch" statement from samples included in CKEditor 5 builds (?) #1803

Closed
wwalc opened this issue Jun 7, 2019 · 6 comments · Fixed by ckeditor/ckeditor5-build-classic#93
Assignees
Labels
status:discussion type:docs This issue reports a task related to documentation (e.g. an idea for a guide). type:improvement This issue reports a possible enhancement of an existing feature.

Comments

@wwalc
Copy link
Member

wwalc commented Jun 7, 2019

I recently encountered an issue when trying to load one of 3rd party plugins into a CKEditor 5 build. Unfortunately the error logged in the console was completely useless:

Screen Shot 2019-06-07 at 11 24 45 AM

Only after I commented the catch block, it showed me something meaningful:

Screen Shot 2019-06-07 at 11 26 01 AM

It took me a few seconds to understand that I can try getting rid of the catch statement that looked like something that should just show me the correct stack trace, to see if that changes anything:

Screen Shot 2019-06-07 at 11 26 35 AM

In order to improve the developer experience, I'd get rid of that statement.

@Mgsy Mgsy added status:discussion type:improvement This issue reports a possible enhancement of an existing feature. labels Jun 10, 2019
@Reinmar
Copy link
Member

Reinmar commented Jun 24, 2019

That's a good catch. We could remove the catch() calls but:

  1. we'd have to check that an unhandled promise error is logged to the console on all browsers (I think it wasn't true back in the days),
  2. it's a good practice to have the .catch() call at the end of a promise chain, so I'm not sure how lack of it will be seen by the developers – it'd be good how others do that in their docs.

@Reinmar Reinmar added this to the iteration 25 milestone Jun 24, 2019
@jodator
Copy link
Contributor

jodator commented Jun 24, 2019

2\. it's a good practice to have the `.catch()` call at the end of a promise chain, so I'm not sure how lack of it will be seen by the developers

Well to be honest I don't what's worse dummy catch or no .catch() clause at all ;)

Maybe the better solution would be (ps.: remember about the full names, like in error?):

.catch( error => {
    // Handle the editor initalisation errors - ... (or some better text :P )
    console.error( 'There was a problem with initializing the editor', error );
} );

with the above we have: full error logged, no uncaught error in promise logged and better practice with having he catch() clause.

But logging the full error will be better then logging only err.stack.

@Reinmar
Copy link
Member

Reinmar commented Jun 24, 2019

But logging the full error will be better then logging only err.stack.

That's the point of:

2. it's a good practice to have the .catch() call at the end of a promise chain, so I'm not sure how lack of it will be seen by the developers

It doesn't work now, but perhaps it can work. The flaws of the current body of the catch() callback are (perhaps) not a reason to completely negate the value of using that catch(). Let's just find out if there's a better way.

@jodator
Copy link
Contributor

jodator commented Jun 24, 2019

Let's just find out if there's a better way.

Logging the full error object instead of error.stack as a start. Adding some message to this might be also helpful (to show that we caught that error).

@wwalc
Copy link
Member Author

wwalc commented Jun 24, 2019

Getting rid of the catch statement was the first idea that came to my mind to solve the real problem that was introduced by "wrong" catch statement. I do agree with both of you that if we can catch errors but at the same time show the full information about the issue (log the full error, whatever) + a custom error message as proposed in #1803 (comment), then it would look more professional, then just getting rid of the catch statement.

@mlewand
Copy link
Contributor

mlewand commented Dec 3, 2019

Fixed with a PR in each build:

oleq added a commit to ckeditor/ckeditor5-build-decoupled-document that referenced this issue Dec 3, 2019
Docs: Error messages in manual tests and a sample should be more verbose should the editor fail to initialize (see ckeditor/ckeditor5#1803).
oleq added a commit to ckeditor/ckeditor5-build-inline that referenced this issue Dec 3, 2019
Docs: Error messages in manual tests and a sample should be more verbose should the editor fail to initialize (see ckeditor/ckeditor5#1803).
oleq added a commit to ckeditor/ckeditor5-build-balloon that referenced this issue Dec 3, 2019
Docs: Error messages in manual tests and a sample should be more verbose should the editor fail to initialize (see ckeditor/ckeditor5#1803).
oleq added a commit to ckeditor/ckeditor5-build-classic that referenced this issue Dec 3, 2019
Docs: Error messages in manual tests and a sample should be more verbose should the editor fail to initialize (see ckeditor/ckeditor5#1803).
oleq added a commit to ckeditor/ckeditor5-build-balloon-block that referenced this issue Dec 3, 2019
Docs: Error messages in manual tests and a sample should be more verbose should the editor fail to initialize (see ckeditor/ckeditor5#1803).
@oleq oleq added the type:docs This issue reports a task related to documentation (e.g. an idea for a guide). label Dec 3, 2019
JDinABox pushed a commit to JDinABox/ckeditor5-build-markdown that referenced this issue Sep 6, 2021
Docs: Error messages in manual tests and a sample should be more verbose should the editor fail to initialize (see ckeditor/ckeditor5#1803).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:discussion type:docs This issue reports a task related to documentation (e.g. an idea for a guide). type:improvement This issue reports a possible enhancement of an existing feature.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants