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

Add more info to crash reports #1253

Merged
merged 19 commits into from
Apr 19, 2024
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 17 additions & 40 deletions packages/php-wasm/logger/src/lib/logger.ts
Original file line number Diff line number Diff line change
Expand Up @@ -120,19 +120,6 @@ export class Logger extends EventTarget {
* The service worker will send the number of open Playground tabs.
*/
public addServiceWorkerMessageListener() {
const getServiceWorkerMetrics = () => {
if (!navigator.serviceWorker.controller) {
return;
}
navigator.serviceWorker.controller.postMessage(
'getServiceWorkerMetrics'
);
};
getServiceWorkerMetrics();
navigator.serviceWorker.addEventListener(
'controllerchange',
getServiceWorkerMetrics
);
navigator.serviceWorker.addEventListener('message', (event) => {
if (event.data?.numberOfOpenPlaygroundTabs !== undefined) {
this.addContext({
Expand Down Expand Up @@ -307,34 +294,24 @@ export function collectPhpLogs(
* @param worker The service worker
*/
export function reportServiceWorkerMetrics(worker: ServiceWorkerGlobalScope) {
worker.addEventListener('activate', (event) => {
// Trigger controllerchange event to update the client count.
/**
* Triggers the `controllerchange` event in other clients to update the count.
*
* > When a service worker is initially registered, pages won't use it until they next load.
* > The claim() method causes those pages to be controlled immediately. Be aware that
* > this results in your service worker controlling pages that loaded regularly over the network,
* > or possibly via a different service worker.
*
* @see https://developer.mozilla.org/en-US/docs/Web/API/Clients/claim
*/
event.waitUntil(worker.clients.claim());
});
worker.addEventListener('message', (event) => {
if (!event.source) {
return;
}
if (event.data === 'getServiceWorkerMetrics') {
worker.clients.matchAll().then((clients) => {
event.source!.postMessage({
numberOfOpenPlaygroundTabs: clients.filter(
// Only count top-level frames to get the number of tabs.
(c) => c.frameType === 'top-level'
).length,
worker.addEventListener('activate', () => {
worker.clients.matchAll().then((clients) => {
const metrics = {
numberOfOpenPlaygroundTabs: clients.filter(
// Only count top-level frames to get the number of tabs.
(c) => c.frameType === 'top-level'
).length,
};
worker.clients
.matchAll({
includeUncontrolled: true,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why includeUncontrolled? Let's leave a comment in the code to explain. Otherwise LGTM 👍

Copy link
Collaborator

Choose a reason for hiding this comment

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

@bgrgicak yeah I meant more not the literal meaning of that option but rather why bother with uncontrolled clients.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can update it in #1264 with something like this.

// Inform all Playground clients that there was a change in the number of active tabs

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right, but why an uncontrolled client is considered a Playground client? It seems like that browser tab wouldn't be able to access Playground anyway, as being controlled is crucial for that, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Correct, I tested this a bit and it's actually not needed, so I will remove this line in #1264

})
.then((clients) => {
for (const client of clients) {
client.postMessage(metrics);
}
});
});
}
});
});
}

Expand Down
Loading