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

Introduce subscription IDs #828

Merged
merged 42 commits into from
Dec 20, 2024
Merged

Introduce subscription IDs #828

merged 42 commits into from
Dec 20, 2024

Conversation

OrKoN
Copy link
Contributor

@OrKoN OrKoN commented Dec 4, 2024

This PR implements subscription IDs which can be used to unsubscribe from events. The specification is refactored
to operate on a list of subscriptions.


Preview | Diff

@OrKoN OrKoN force-pushed the orkon/flat-subscribe branch 2 times, most recently from 89afc76 to 722f856 Compare December 4, 2024 17:04
@OrKoN
Copy link
Contributor Author

OrKoN commented Dec 4, 2024

@jgraham I started a draft for managing subscriptions using a single list. I hope it is somewhat readable and if you think it is going in the right direction, I will work on TODOs and trying to make it landable.

Copy link
Member

@jgraham jgraham left a comment

Choose a reason for hiding this comment

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

In general I think this looks like the right approach, thanks!

We probably need to think carefully about the data structure for subscriptions, but I think it's going to work out :)

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@OrKoN OrKoN force-pushed the orkon/flat-subscribe branch from 722f856 to e9362fe Compare December 9, 2024 09:00
@OrKoN OrKoN changed the title Subscription IDs and subscription list Introduce subscription IDs Dec 10, 2024
Copy link
Member

@jgraham jgraham left a comment

Choose a reason for hiding this comment

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

Generally I think this is correct, but some of the use of spec primitives needs to be tightened up.

Also I think this model where we start with maximally compact structs with sets of both events and contexts, but in unsubscription break them apart into one struct per event is an awkward tradeoff

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@OrKoN OrKoN force-pushed the orkon/flat-subscribe branch 2 times, most recently from 50c1eb3 to fda1c58 Compare December 16, 2024 10:22
@OrKoN OrKoN requested a review from jgraham December 17, 2024 07:14
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@OrKoN OrKoN force-pushed the orkon/flat-subscribe branch 3 times, most recently from aba4af5 to e28e69f Compare December 18, 2024 18:09
OrKoN and others added 23 commits December 20, 2024 11:55
Co-authored-by: Maksim Sadym <69349599+sadym-chromium@users.noreply.github.com>
Co-authored-by: Maksim Sadym <69349599+sadym-chromium@users.noreply.github.com>
Co-authored-by: Maksim Sadym <69349599+sadym-chromium@users.noreply.github.com>
Co-authored-by: Maksim Sadym <69349599+sadym-chromium@users.noreply.github.com>
Co-authored-by: Maksim Sadym <69349599+sadym-chromium@users.noreply.github.com>
Co-authored-by: Maksim Sadym <69349599+sadym-chromium@users.noreply.github.com>
Co-authored-by: Maksim Sadym <69349599+sadym-chromium@users.noreply.github.com>
Co-authored-by: Maksim Sadym <69349599+sadym-chromium@users.noreply.github.com>
Co-authored-by: Maksim Sadym <69349599+sadym-chromium@users.noreply.github.com>
Co-authored-by: Maksim Sadym <69349599+sadym-chromium@users.noreply.github.com>
Co-authored-by: jgraham <james@hoppipolla.co.uk>
Co-authored-by: jgraham <james@hoppipolla.co.uk>
@OrKoN OrKoN force-pushed the orkon/flat-subscribe branch from fc147ba to a6cae3f Compare December 20, 2024 10:56
@OrKoN OrKoN merged commit 60b9cc8 into main Dec 20, 2024
5 checks passed
@OrKoN OrKoN deleted the orkon/flat-subscribe branch December 20, 2024 13:27
@OrKoN
Copy link
Contributor Author

OrKoN commented Dec 20, 2024

I filed GoogleChromeLabs/chromium-bidi#2925 to track the work on implementation and WPT

github-actions bot added a commit that referenced this pull request Dec 20, 2024
SHA: 60b9cc8
Reason: push, by OrKoN

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants