-
Notifications
You must be signed in to change notification settings - Fork 268
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
Add fatal errror listener #1095
Merged
Merged
Changes from all commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
669757e
Collect request errors
bgrgicak da44dd0
Remove service worker logs
bgrgicak f295b8c
Add fatal error event
bgrgicak 24f4d0d
Fix base-php error
bgrgicak c3d4941
Add event listener
bgrgicak 6520a2b
Fix linter errors
bgrgicak f447ca9
Remove debug code
bgrgicak f8694b8
Use custom event target instead of window
bgrgicak 93e28c0
Inline dispatch event method
bgrgicak be0148c
Merge branch 'trunk' into add/logger-crash-event
bgrgicak a9aa605
Add event listener tests
bgrgicak 8f5dab5
Add missing polyfills
bgrgicak 91a5e72
Improve listener description
bgrgicak 539a5ec
Merge branch 'trunk' into add/logger-crash-event
bgrgicak 6488287
Merge branch 'trunk' into add/logger-crash-event
bgrgicak cb74fb3
Merge branch 'trunk' into add/logger-crash-event
bgrgicak File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1,4 @@ | ||
export * from './logger'; | ||
// PHP.wasm requires WordPress Playground's Node polyfills. | ||
import '@php-wasm/node-polyfills'; | ||
|
||
export * from './lib/logger'; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
import { NodePHP } from '@php-wasm/node'; | ||
import { LatestSupportedPHPVersion } from '@php-wasm/universal'; | ||
import { logger, addFatalErrorListener, collectPhpLogs } from '../lib/logger'; | ||
|
||
describe('Logger', () => { | ||
let php: NodePHP; | ||
beforeEach(async () => { | ||
php = await NodePHP.load(LatestSupportedPHPVersion); | ||
}); | ||
it('Event listener should work', () => { | ||
const listener = vi.fn(); | ||
collectPhpLogs(logger, php); | ||
addFatalErrorListener(logger, listener); | ||
php.dispatchEvent({ | ||
type: 'request.error', | ||
error: new Error('test'), | ||
}); | ||
expect(listener).toBeCalledTimes(1); | ||
|
||
const logs = logger.getLogs(); | ||
expect(logs.length).toBe(1); | ||
}); | ||
}); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,12 +22,14 @@ import { acquireOAuthTokenIfNeeded } from './github/acquire-oauth-token-if-neede | |
import { GithubImportModal } from './github/github-import-form'; | ||
import { GithubExportMenuItem } from './components/toolbar-buttons/github-export-menu-item'; | ||
import { GithubExportModal } from './github/github-export-form'; | ||
import { useEffect, useState } from 'react'; | ||
import { useState } from 'react'; | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Related discussion #1035 (comment) |
||
|
||
const query = new URL(document.location.href).searchParams; | ||
const blueprint = await resolveBlueprint(); | ||
|
||
|
@@ -83,10 +85,6 @@ function Main() { | |
Partial<ExportFormValues> | ||
>({}); | ||
|
||
useEffect(() => { | ||
collectWindowErrors(logger); | ||
}, []); | ||
|
||
return ( | ||
<PlaygroundContext.Provider value={{ storage }}> | ||
<PlaygroundViewport | ||
|
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 totrue
– that's probably out of scope for this PR.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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.