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

Proposal to remove non-global unsubscribe requests for events #829

Open
OrKoN opened this issue Dec 10, 2024 · 9 comments
Open

Proposal to remove non-global unsubscribe requests for events #829

OrKoN opened this issue Dec 10, 2024 · 9 comments
Labels
needs-discussion Issues to be discussed by the working group

Comments

@OrKoN
Copy link
Contributor

OrKoN commented Dec 10, 2024

With #828, it will be possible to unsubscribe from specific subscriptions by ID.

Currently, the unsubscribe command can accept subscription requests meaning that it supports global unsubscribe requests (i.e, defining only event names, meaning all previous subscriptions for these events will be unsubscribed) and context unsubscribe requests (i.e., requests supplying both event names and context ids). One particular drawback is that counter-intuitively a non-global unsubscribe request does not have any effect on the global subscriptions (and supporting it is not feasible since it would require subscriptions that are able to subscribe to all contexts except the listed ones).

We propose removing the support for non-global unsubscribe events in the future. While it is a breaking change, I think it will reduce the complexity of the specification (especially, when more configuration options are added to subscriptions) and would not be a breaking change for most clients (Puppeteer, Selenium, WebDriverIO, WPT do not seem to adopt non-global unsubscribe requests).

@shs96c @christian-bromann do you know if Selenium and WebDriverIO rely on non-global unsubscribe events (i.e., session.unsubscribe(contexts=[non empty list of contexts], events=[non empty list of events])?

@OrKoN OrKoN added the needs-discussion Issues to be discussed by the working group label Dec 10, 2024
@jgraham
Copy link
Member

jgraham commented Dec 10, 2024

Aslo @jimevans and we also know that browsertime and Cypress are using BiDi. I think we're past the point where we can confidently survey "all" users.

@jimevans
Copy link
Collaborator

@jgraham With Selenium, not all language bindings have equally had features migrated to BiDi, but I only see one place in the code base (in one language binding) where session.unsubscribe is called with both a list of contexts and a list of events. I think that particular instance of calling that possibly should be a global unsubscribe.

@OrKoN
Copy link
Contributor Author

OrKoN commented Dec 11, 2024

Based on cypress-io/cypress#30447 it seems Cypress has not adopted WebDriver BiDi yet.

@OrKoN
Copy link
Contributor Author

OrKoN commented Dec 11, 2024

@jimevans do you happen to still have a link to that instance?

@jgraham
Copy link
Member

jgraham commented Dec 11, 2024

Maybe @AtofStryker can weigh in on Cypress. I believe they already have a branch, and I don't want to shift the goalposts under them by agreeing to remove a feature they're using.

@jimevans
Copy link
Collaborator

@OrKoN
Copy link
Contributor Author

OrKoN commented Dec 11, 2024

@OrKoN Sure. The only place I see it used that way is https://github.com/SeleniumHQ/selenium/blob/f697509758cad3589daf04a02420ece3c3752c54/javascript/node/selenium-webdriver/bidi/logInspector.js#L342

thanks, I think this part can benefit from using subscription IDs so that it does not require if/else when unsubscribing.

@css-meeting-bot
Copy link
Member

The Browser Testing and Tools Working Group just discussed Proposal to remove non-global unsubscribe requests for events.

The full IRC log of that discussion <AutomatedTester> topic: Proposal to remove non-global unsubscribe requests for events
<AutomatedTester> github: https://github.com//issues/829
<AutomatedTester> orkon: I was working on the change to allow unsubscribe by IDs so ahve come up mwith this proposal
<AutomatedTester> ... there is a non-global unsubscribve request
<AutomatedTester> ... if we want to support it we are going to ahve to split the subscriptions into multiple ways
<AutomatedTester> ... a user can subscribe globally and then unsubscribe by ID then nothing is actually going to happen
<jgraham> q+
<AutomatedTester> ... this proposal will simplify the way people use it and simplify the spec
<AutomatedTester> ack next
<AutomatedTester> jgraham: I agree this is confusing
<orkon> q+
<AutomatedTester> ... it's not undefined. It may do in some cases. I am not sure about removing it as we don't have a way of checking who is using it
<AutomatedTester> ... I think we can put ourselves on a path to removing it with warning but not just remove it yet
<AutomatedTester> ... it would have been better doing it 2 years ago but we didn't.
<jdescottes> q+
<AutomatedTester> ack next
<AutomatedTester> orkon: I have a specific proposal is to define a data structure that won't go into the way of it with a warning
<jdescottes> q-
<AutomatedTester> ... if there are clients that are broken we can always go help them there
<jgraham> q+
<jdescottes> q+
<AutomatedTester> ... but I feel there won't be many clients affected
<AutomatedTester> ack next
<AutomatedTester> jgraham: that says reasonable
<AutomatedTester> ack next
<orkon> q+
<AutomatedTester> jdescottes: this sounds good to me. For the spec changes, do you think we can do the old way and do the way with IDs?
<AutomatedTester> ... not sure if you have put thought into that?
<AutomatedTester> ack next
<AutomatedTester> orkon: I have the PR that jgraham has reviewed it partially. The issue is to unsubscrube to partial subscriptions
<AutomatedTester> ... jgraham has put one model in the issue
<AutomatedTester> ... <missed the 2nd way>
<AutomatedTester> q?

@AtofStryker
Copy link

Maybe @AtofStryker can weigh in on Cypress. I believe they already have a branch, and I don't want to shift the goalposts under them by agreeing to remove a feature they're using.

Thanks for the tag @jgraham. I don't think we have the need to actually unsubscribe from anything. We just subscribe to the events we need in the session here in our spike investigation to cut over to BiDi.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-discussion Issues to be discussed by the working group
Projects
None yet
Development

No branches or pull requests

5 participants