Add logging support to Playground - #1035
Conversation
|
@adamziel this is still missing proper Playground log collecting, but it will help debugging WordPress as-is. The logger is now loaded in Blueprints because this is the first place I found that exposes I wanted to keep the logger tied to a single package for this PR, but it would be great if we could move it to a separate package and call it in different packages as needed. For example, we can't detect the |
I guess we could move it to Remote. |
| /** | ||
| * Collect errors from JavaScript window events like error and log them. | ||
| */ | ||
| private collectJavaScriptErrors() { |
There was a problem hiding this comment.
It would be nice to make this pluggable, maybe as a Constructor argument? This way different runtimes could ship their own global error collecting logic, e.g. the VS Code extension will need highly customized logic that I don't think belongs to this repo. Also, there might be environments where the window object exists, and yet it either doesn't provide any useful error information or only provides Playground-unrelated noise – I vaguely remember VS Code might be one of them.
There was a problem hiding this comment.
Is it necessary? What would the constructor argument do?
The logger should be accessible by all environments, so a runtime can submit logs and when we add full support it will also be able to read logs.
There was a problem hiding this comment.
Is it necessary?
We can merge without that. I'd just eventually move the responsibility for selecting the logging method to the consumer of this API instead of guessing what the environment is.
On the website we'd call something like collectWindowErrors(), in web worker that could be collectWorkerErrors(), and then there could be bindings for service workers, Node workers etc.
My point was mostly about exposing a loosely coupled API like collectWindowErrors(logger) or collectPlaygroundErrors(logger, playground); vs tightly coupled logger.collectWindowErrors() logger.addPlaygroundRequestEndListener(). This would make the Logger class lean and responsible for a single thing, while the "error collectors" could be plentiful, separate, and also provided by other modules.
There was a problem hiding this comment.
I refactored the code to match what you described in a3feb32.
Let me know what you think.
collectWindowErrors runs only in the Website package now.
| }), | ||
| progressTracker | ||
| ); | ||
| getLogger().addPlaygroundRequestEndListener(playground); |
There was a problem hiding this comment.
Nitpick for iteration 2: We could import an instantiated logger instead of a getLogger() function.
import { logger } from '@php-wasm/logger';
logger.addPlaygroundRequestEndListener(playground);
There was a problem hiding this comment.
Oh! This is actually blocking: the logger should only collect all the window errors on playground.wordpress.net in the context of the website app. However, the client package is a universally reusable artifact that other websites and app can reuse, e.g. https://translate.wordpress.org/projects/wp-plugins/friends/dev/de/default/playground/. do this:
import { startPlaygroundWeb, phpVars, setPluginProxyURL } from 'https://playground.wordpress.net/client/index.js';Calling this.collectJavaScriptErrors(); in the logger constructor would make the logger collect all the Playground-unrelated global JS errors in all the apps that merely use the client.
This is why I wouldn't attach any global error collection logic (e.g. window-related) at the library level but defer it to the consumer app. This way, Playground worker-thread, remote/index.html and website could all decide to attach global error listeners without simultaneously imposing that decision on the API consumers.
That being said, binding the logger to the Playground instance here seems great 👍
|
@bgrgicak I left a bunch of nitpicks that are fine to address in a subsequent PR, but I also found one blocker: |
ecb1376 to
ef4993b
Compare
adamziel
left a comment
There was a problem hiding this comment.
This looks good, thank you for working through all the feedback @bgrgicak! I left one last note in https://github.com/WordPress/wordpress-playground/pull/1035/files#r1506127419, and once that's addressed I'm happy to get this in!
| * @param loggerInstance The logger instance | ||
| */ | ||
| export function collectWindowErrors(loggerInstance: Logger) { | ||
| loggerInstance.addWindowErrorListener(); |
There was a problem hiding this comment.
Perhaps eventually this call could be inlined, as in:
export function collectWindowErrors(loggerInstance: Logger) {
window.addEventListener('error', e => logWindowError(loggerInstance, e));
window.addEventListener(
'unhandledrejection',
e => logUnhandledRejection(loggerInstance, e.reason)
);
window.addEventListener(
'rejectionhandled',
e => logUnhandledRejection(loggerInstance, e.reason)
);
}But for now, let's leave it the way it is and see how adding support for other runtimes shapes the development of the API.
|
I moved the log collection to main. |
| >({}); | ||
|
|
||
| useEffect(() => { | ||
| collectWindowErrors(logger); |
There was a problem hiding this comment.
This could even be a top-level call outside of the Main component , e.g. right after the imports – this way it would also collect all the errors that happened before React had a chance to render.
There was a problem hiding this comment.
I can move it in the next pr.
|
Yay, I'm really glad to have this PR in! |
What is this PR doing?
This PR adds support for collecting PHP logs and WordPress debug logs in Playground.
After each request logs are printed with
console.debugin the browser.Support for Node, WP-now, and VSCode will be added in separate PRs.
We also plan to add more Playground logs in the future to help us improve the stability of WordPress Playground.
What problem is it solving?
When Playground crashes it's hard to know why it happened. By adding access to PHP logs and users will be able to debug their sites.
Playground developers will benefit from having Playground and PHP logs in a single place and a shared format.
How is the problem addressed?
On
request.endwe collect logs from the current PHP request and send them back with the event.These logs are caught by the Logger and printed.
Additionally, the logger catches some of the Playground errors using regular
windowevents.Testing Instructions
npm run devthrow new Exception('This is a test exception');after this file ofwordpress-playground/packages/playground/remote/src/lib/playground-mu-plugin/playground-includes/playground_logger.php
Line 9 in c23dd92