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: Event.addListener should accept an interface. #519

Open
newRoland opened this issue Jan 8, 2024 · 8 comments
Open

Proposal: Event.addListener should accept an interface. #519

newRoland opened this issue Jan 8, 2024 · 8 comments
Labels
enhancement Enhancement or change to an existing feature proposal Proposal for a change or new feature topic: events Issues related to the registration, management, and dispatch of WebExtension events.

Comments

@newRoland
Copy link

newRoland commented Jan 8, 2024

addListener accepts a callback function as the first argument. The subsequent arguments are used for event-specific types. For example, webRequest.onAuthRequired.addListener takes a RequestFilterfor the second argument, and extraInfoSpec for the third.

There's no place to provide parameters that are relevant to all events. Allowing an interface for the first argument would provide the flexibility for future use cases and proposals. For example, I make use of this hypothetical interface in my #501 proposal.

browser.storage.local.onChanged.addListener(onChanged)

// flexible form allows for future use cases
browser.storage.local.onChanged.addListener({
    callback: onChanged
})
@tophf
Copy link

tophf commented Jan 8, 2024

Maybe use handleEvent instead of callback to match DOM addEventListener?

@newRoland
Copy link
Author

This must be null, an object with a handleEvent() method, or a JavaScript function.

I learned something new today, didn't know that was a thing.

@dotproto
Copy link
Member

dotproto commented Jan 10, 2024

There's no place to provide parameters that are relevant to all events

I don't see why this specific solution would be the best way to address common parameters given the current design of WebExtensions APIs. It seems like we could accomplish something similar by having common parameters in an optional property bag. As you mentioned, a number of existing event listener already have the equivalent of an options parameter that contains a number of key-value pairs to control listener behavior. This feels to me like a better fit given the way event registration is currently handled in extensions APIs.

It's also worth noting that the web uses a different approach to what this issue proposes for common properties associated with all events; the web uses an options object on EventTarget.addEventListener() for things like providing an AbortController or signaling if the listener is passive.

Beyond that, I'm also very hesitant about changing how Event.addListener() behaves. Adding new behavior to all existing event listeners is a nontrivial amount of work and would increase the maintenance cost for browser engineers. In order to tackle something like that, I suspect that vendors will want to see substantial benefits to the new approach and possibly a deprecation plan for the the older approach. I am also concerned about the challenges related to supporting two different registration patterns on the same method names. To my knowledge browser engineers strongly prefer not to overload method signatures where possible.

All that said, I'm very interested in cleaning up how extension event listeners are registered and managed. I would like to explore what the platform would look and feel like if we used an event model that's more in line with the web platform. Specifically, things like using potentially using the EventTarget interface for extension namespaces. But that's all a bit tangential to this specific proposal.

@newRoland
Copy link
Author

newRoland commented Jan 10, 2024

I'm interesting in having a designated area where you can supply parameters that might be relevant to all events. If the 2nd parameter makes more sense, that's also fine.

Beyond that, I'm also very hesitant about changing how Event.addListener() behaves.

My intent is to make it easier to change how the addListener behaves. If there's no appetite for that, I think this is a dead end proposal. I'll close it for now and mull it over. Thank you for the detailed reply.

@newRoland newRoland closed this as not planned Won't fix, can't repro, duplicate, stale Jan 10, 2024
@dotproto
Copy link
Member

My intent is to make it easier to change how the addListener behaves. If there's no appetite for that, I think this is a dead end proposal.

At this point I don't think we know how much appetite there is. I have concerns, but I don't speak for any browsers. It might be worth discussing this idea in a meeting to get a sense of how this approach strikes the browser reps. Even if they raise concerns, that conversation may help inform what we explore next.

@dotproto dotproto added enhancement Enhancement or change to an existing feature topic: events Issues related to the registration, management, and dispatch of WebExtension events. proposal Proposal for a change or new feature and removed needs-triage labels Jan 10, 2024
@newRoland newRoland reopened this Jan 11, 2024
@jpmedley
Copy link

Specifically, things like using potentially using the EventTarget interface for extension namespaces. But that's all a bit tangential to this specific proposal.

Would this create a situation where documentation would constantly be saying this such as: like an interface except or like a namespace except. I'm being impressionistic because I'm not sure those exact words would be used. I'm trying to make the point that exceptions to existing patterns can be stumbling blocks to users even when they're adequately documented.

@tophf
Copy link

tophf commented Jan 22, 2024

addEventListener accepts an interface for decades(?) and how many developers are confused by its documentation or implementation? I think it's probably 0 because this edge case is ignored by anyone who is not looking for it specifically. So it's possible to document/implement this functionality without creating unnecessary confusion.

@jpmedley
Copy link

I was about to elaborate, but realized I overlooked part of Simeon's statement. "But that's all a bit tangential to this specific proposal." I'll comment again if a specific proposal is made.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement or change to an existing feature proposal Proposal for a change or new feature topic: events Issues related to the registration, management, and dispatch of WebExtension events.
Projects
None yet
Development

No branches or pull requests

4 participants