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

Make displayed Web errors more meaningful #92553

Merged
merged 1 commit into from
May 31, 2024

Conversation

adamscott
Copy link
Member

Updates the code to display an error to not only show the message, but the context as well.

Before After
Capture d’écran, le 2024-05-30 à 10 58 26 Capture d’écran, le 2024-05-30 à 10 59 21

@adamscott adamscott added this to the 4.3 milestone May 30, 2024
@adamscott adamscott requested a review from a team May 30, 2024 15:08
@adamscott adamscott requested a review from a team as a code owner May 30, 2024 15:08
Copy link
Collaborator

@Faless Faless left a comment

Choose a reason for hiding this comment

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

Looking good! Nicely spotted!

@patwork
Copy link
Contributor

patwork commented May 30, 2024

Are we ok with "??" https://caniuse.com/?search=%3F%3F

Without need for msg it could be even:

	function displayFailureNotice(err) {
		console.error(err);
		setStatusNotice(err.message ?? err);
		setStatusMode('notice');
		initializing = false;
	}

will it make a difference?

setStatusNotice(err.message || err);

💣 EDIT: setStatusNotice will not handle err properly if it is not a string (because of split('\n'))

@Faless
Copy link
Collaborator

Faless commented May 30, 2024

Are we ok with "??" https://caniuse.com/?search=%3F%3F

Indeed, I missed that, what's the rationale for chaining to ???

@adamscott
Copy link
Member Author

Are we ok with "??" https://caniuse.com/?search=%3F%3F

Indeed, I missed that, what's the rationale for chaining to ???

?? is the new standard to test if a value exists or not. In the code changed, if err.message === "", it will resort back to err even if "" is a valid message, all because "" is a falsy value (which the or tests). The nullish operator (which is pretty much supported by all browsers since 2020) tests only if it's nullish, not if it's falsy.

@patwork
Copy link
Contributor

patwork commented May 30, 2024

Ah yes, ?? is better in this case, because:

const err = { message: "" };
const msg = err.message ?? err;
console.log(msg); // Outputs: ""

and || will break setStatusNotice

const err = { message: "" };
const msg = err.message || err;
console.log(msg); // Outputs: { message: "" }

@adamscott adamscott force-pushed the more-meaningful-errors branch 2 times, most recently from e3583a4 to 87a6a9f Compare May 30, 2024 15:51
@adamscott
Copy link
Member Author

adamscott commented May 30, 2024

@patwork @Faless I updated my code finally, to make sure that setStatusNotice() is well set. Maybe overkill, but it will work.

function displayFailureNotice(err) {
	console.error(err);
	if (err instanceof Error) {
		setStatusNotice(err.message);
	} else if (typeof err === 'string') {
		setStatusNotice(err);
	} else if (typeof err === 'object' && typeof err.toString === 'function') {
		setStatusNotice(err.toString());
	} else {
		setStatusNotice('An unknown error occured');
	}
	setStatusMode('notice');
	initializing = false;
}

@patwork
Copy link
Contributor

patwork commented May 30, 2024

I guess we can assume that err will be either string or Error().

This could potentially get us "[Object Object]" or similiar:

	} else if (typeof err === 'object' && typeof err.toString === 'function') {
		setStatusNotice(err.toString());

It is better to write right away that it is an unknown error.

a= new Promise(() => {});
typeof a;
"object"
a.toString();
"[object Promise]" 

@akien-mga akien-mga merged commit cf9af1e into godotengine:master May 31, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants