Add fatal errror listener #1095
Conversation
| @@ -292,6 +292,12 @@ export abstract class BasePHP implements IsomorphicLocalPHP { | |||
| throw error; | |||
| } | |||
| return response; | |||
| } catch (e) { | |||
| this.dispatchEvent({ | |||
There was a problem hiding this comment.
Cool! Note this might either signify a fatal error in PHP, if request.throwOnError is true, or a more serious issue like, e.g., Asyncify exception.
There was a problem hiding this comment.
Would it be useful to also emit response information when available? Also, I wonder whether we should remove throwOnError and always act as if it was set to true – that's probably out of scope for this PR.
There was a problem hiding this comment.
Note this might either signify a fatal error in PHP, if request.throwOnError is true, or a more serious issue like, e.g., Asyncify exception.
Thanks! Both are valid cases for returning the event, but it would be good to distinguish between them.
There was a problem hiding this comment.
Also, I wonder whether we should remove throwOnError and always act as if it was set to true – that's probably out of scope for this PR.
Let's take a look at this separately. I'm trying to avoid changing current logs in this project to stay focused.
But there is definitely room for improvement in how we throw and display errors.
There was a problem hiding this comment.
Would it be useful to also emit response information when available?
Do you mean emit the response when the request is successful?
I don't know how I would do it for failed requests. There is #getResponseHeaders for getting the headers, but I'm not sure how useful that is.
There was a problem hiding this comment.
PHP exceptions won't trigger this. Only internal errors like Asyncify.
For some reason, I thought they would, but after testing it, it doesn't.
You get a WordPress error screen and can read the log in the browser console/debug.log.
There was a problem hiding this comment.
@bgrgicak actually, this will be triggered by PHP exceptions if this method is called with throwOnError, see the line 292. Let's fix that.
There was a problem hiding this comment.
I'm unable to recreate this. Do you have an example?
I tried throwing an error in 0-playground.php and introducing syntax errors. Both just returned a 500, but didn't trigger the event.
There was a problem hiding this comment.
php.addEventListener('request.error', (e) => {
console.log("Request error");
});
php.run({
throwOnError: true,
code: `<?php
syntax_error
`,
});There was a problem hiding this comment.
Thanks! I added a flag that will prevent the modal from triggering.
There is still value in catching these errors in some cases. For example if a developer uses Playground to demo their product and wants to know when it breaks on Playground.
Playground.WordPress.net won't show the modal, but a developer might decide to log the message somewhere to improve their demo experience.
| import { ExportFormValues } from './github/github-export-form/form'; | ||
| import { joinPaths } from '@php-wasm/util'; | ||
| import { PlaygroundContext } from './playground-context'; | ||
| import { collectWindowErrors, logger } from '@php-wasm/logger'; | ||
|
|
||
| collectWindowErrors(logger); |
There was a problem hiding this comment.
Related discussion #1035 (comment)
| @@ -292,6 +292,12 @@ export abstract class BasePHP implements IsomorphicLocalPHP { | |||
| throw error; | |||
| } | |||
| return response; | |||
| } catch (e) { | |||
| this.dispatchEvent({ | |||
There was a problem hiding this comment.
Note this might either signify a fatal error in PHP, if request.throwOnError is true, or a more serious issue like, e.g., Asyncify exception.
Thanks! Both are valid cases for returning the event, but it would be good to distinguish between them.
| @@ -292,6 +292,12 @@ export abstract class BasePHP implements IsomorphicLocalPHP { | |||
| throw error; | |||
| } | |||
| return response; | |||
| } catch (e) { | |||
| this.dispatchEvent({ | |||
There was a problem hiding this comment.
Also, I wonder whether we should remove throwOnError and always act as if it was set to true – that's probably out of scope for this PR.
Let's take a look at this separately. I'm trying to avoid changing current logs in this project to stay focused.
But there is definitely room for improvement in how we throw and display errors.
| @@ -292,6 +292,12 @@ export abstract class BasePHP implements IsomorphicLocalPHP { | |||
| throw error; | |||
| } | |||
| return response; | |||
| } catch (e) { | |||
| this.dispatchEvent({ | |||
There was a problem hiding this comment.
Would it be useful to also emit response information when available?
Do you mean emit the response when the request is successful?
I don't know how I would do it for failed requests. There is #getResponseHeaders for getting the headers, but I'm not sure how useful that is.
|
Let's add at least one unit test just as a smoke test to confirm dispatching CustomEvent works. |
adamziel
added
[Type] Enhancement
bgrgicak
force-pushed
the
add/logger-crash-event
branch
from
9762186 to
93e28c0
Compare
Done a9aa605 |
| @@ -186,3 +240,15 @@ export function collectPhpLogs( | |||
| ) { | |||
| loggerInstance.addPlaygroundRequestEndListener(playground); | |||
| } | |||
|
|
|||
| /** | |||
| * Add a listener for the fatal error event. | |||
There was a problem hiding this comment.
Let's explain what's a fatal error – is it related to WordPress? PHP.wasm? The current JavaScript app? How is it different from window errors?
There was a problem hiding this comment.
I clarified that this includes only Playground errors and not PHP errors. 91a5e72
I also mentioned that logs are available in the event.
|
I just ran |
|
Saving the CI test failures stack traces for posterity – would a rebase solve those? Or is this a new issue? |
|
@adamziel this unit test is also failing for me on trunk. Would it be ok to merge this PR as is and fix the test in another PR? I wouldn't rush the merge, but this feature is blocking adoption. |
|
Issue for the failing unit test #1112 |
What is this PR doing?
It allows developers to listen for fatal Playground errors and collect logs.
What problem is it solving?
This will allow Playground to collect fatal errors from user sessions in the future.
It will also allow developers who use
startPlaygroundWebto listen for errors and create custom workflows.How is the problem addressed?
All logs are now collected inside the logger including PHP request errors.
When a PHP request error happens the logger dispatches a custom event that developers can listen.
Testing Instructions
npm run devpackages/playground/website/src/main.tsx