-
-
Notifications
You must be signed in to change notification settings - Fork 834
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
feat: Graceful frontend extension initialization failure #3349
Conversation
a28d7a2
to
993d6d3
Compare
this.initializers.toArray().forEach((initializer) => { | ||
try { | ||
initializer(this); | ||
} catch (e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we rethrow e
so the original error is accessible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error is still accessible since we display it with console.error
in the introduced helper. (see screenshot above)
If we throw the execution halts and the app will display the nojs content:
framework/framework/core/views/frontend/app.blade.php
Lines 27 to 36 in b64003c
try { | |
flarum.core.app.load(@json($payload)); | |
flarum.core.app.bootExtensions(flarum.extensions); | |
flarum.core.app.boot(); | |
} catch (e) { | |
var error = document.getElementById('flarum-loading-error'); | |
error.innerHTML += document.getElementById('flarum-content').textContent; | |
error.style.display = 'block'; | |
throw e; | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense!
|
||
caughtInitializationErrors.push(() => fireApplicationError({ | ||
userTitle: extractText(app.translator.trans('core.lib.error.extension_initialiation_failed_message', { extension })), | ||
consoleTitle: `${extension} failed to initialize`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why keep this untranslated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't think it would need translation since it's a console message along with English error messages. These will most of the time be copy-pasted to extension authors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with that there's no reason to translate this.
* } | ||
* @param errors | ||
*/ | ||
export default function fireApplicationError(options: { userTitle: string, consoleTitle: string }, ...errors: any) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooo I like this!
Seems to work fine. I do wonder if there's something we could do to make it able to be monkey patched by e.g. Sentry in order to report these errors. Only way these errors would be caught now would be with console logging enabled. Think that's an issue more in general with exporting functions, however. I believe we've had some issues relating to that in the past? Unsure. Just some thoughts. |
Good point for a good use case. But yeah if it were to be done, monkey patching the function itself would be the best approach, which yeah isn't possible right now, but will be once the export registry is tackled, which is on the 1.x roadmap. |
Part of flarum/issue-archive#85
Changes proposed in this pull request:
Instead of allowing the application's frontend to crash, we can catch the frontend errors when initializing extensions and gracefully fail to minimize the impact and improve user experience.
Reviewers should focus on:
The approach, error message formulation, general code quality.
Screenshot
Necessity
Confirmed
composer test
).