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

Log err.message instead of <unavailable> err object #2939

Merged

Conversation

Eselce
Copy link
Contributor

@Eselce Eselce commented Apr 9, 2018

Most of the log messages printing a caught error object err (or chrome.runtime.lastError) were converted to <unavailable>, which is not very helpful. But then err.message provides the information needed.

@Sxderp
Copy link
Contributor

Sxderp commented Apr 9, 2018

As an aside, I think it depends on which console you're looking at. There're three consoles; extension debugging, browser, and page. One of them is bound to actually provide access to the object.

@Eselce
Copy link
Contributor Author

Eselce commented Apr 9, 2018

@Sxderp Yes, that's correct. I played around with exceptions, errors, etc., and there are quite a few occations, when you get a full output on the (tool) Ctrl-Shift-I console (provided, that the filter is set approriately), even with a stack trace. Many useful logs go to the browser (web) Ctrl-Shift-J console, where access is limited. Debugging can provide the object, but that has nothing to do with these log messages.

Maybe there is a case, where we would like to see the whole object (and it has a nice output), but for me, it looks as if I often miss a helpful error message, if an error occurs. Tell me, if there are really many cases (apart from debugging with break points and value evaluation), where you need the whole error object (in the case of err = chrome.runtime.lastError, documentation tells me, that only err.message is guaranteed)...

@arantius
Copy link
Collaborator

arantius commented Apr 9, 2018

Please help me understand/test: Clear reproduction case where the effect before this change is undesirable?

@Eselce
Copy link
Contributor Author

Eselce commented Apr 9, 2018

Please help me understand/test: Clear reproduction case where the effect before this change is undesirable?

I don't know, if I understood your question right, but here is my "problem" with the current output:

Now and then, while testing, an unexpected error occurs. Reason: wrong data, a programming error, sth. weird.
The result may be, that scripts are (silently) not executed, not loaded, not installed.
Then I check the console output(s) to look for error output (which is mostly the case, either on the web or on the tool console). I want a sensible message text for help, but what I get, is something like Error: <unavailable>. Not very helpful. Maybe I could get more information using the debugger, but for most of these errors, I just need a hint, not the error type or the stack trace. It's unnecessary uninformative...

@@ -105,8 +105,8 @@ async function installFromDownloader(userScriptDetails, downloaderDetails) {
}
return details.uuid;
}).catch(err => {
console.error('Error in installFromDownloader()', err);
// Rethrow so caller can also deal with it
console.error('Error in installFromDownloader():', err.message);
Copy link
Contributor

Choose a reason for hiding this comment

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

I could be wrong about this, but I don't think indexedDB errors are guaranteed to have a .message property. Might want to force a ConstraintError (as an example) and see if it has a message. If it doesn't then this may need to be changed to err.message || err.name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This could be a good idea for every such error message (except for chrome.runtime.lastError).

@arantius
Copy link
Collaborator

Sorry I forgot about this for a while, now it won't merge cleanly.

@arantius arantius added this to the 4.8 milestone Aug 22, 2018
arantius added a commit to arantius/greasemonkey that referenced this pull request May 8, 2019
@arantius arantius merged commit 6829e4d into greasemonkey:master May 8, 2019
@Cerberus-tm
Copy link

@arantius Thank you for all these recent fixes. Very nice.

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.

4 participants