Skip to content

Introduce more general notification capabilities #568

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

karlseguin
Copy link
Collaborator

Replaces the existing, very specialized Notification with something more general.

Currently, the existing page_navigate and page_navigated have been migrated.

Telemetry's page navigation event now also hooks into these events to generate the telemetry record.

There's a comment on the Notification class that talks about scoping, but...

I initially thought I'd have a global singleton, and it would be really easy to register and dispatch events. But then I realized that wouldn't work. If everything's a global, and CDP is registered for "http_bytes_received", then when Telemetry makes an HTTP request, and the client emits a "http_bytes_received" message, CDP will get those messages.

I realized things needed to be scoped. I thought about introducing a "scope_id", but decided to create a notification-per-scope. Currently, there's a Notification per Browser and one Notification for the App. I don't expect more. You can form notification topologies, which is exactly how Telemetry now works. Telemetry listens to the notification_created event from app.notification, and then registers for the page_navigate on the newly created notification instance.

Replaces the existing, very specialized Notification with something more
general.

Currently, the existing page_navigate and page_navigated have been migrated.

Telemetry's page navigation event now also hooks into these events to generate
the telemetry record.
@karlseguin
Copy link
Collaborator Author

I'd rather merge this after the isolated world changes are settled.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant