WICG / observable

Observable API proposal
https://wicg.github.io/observable/
Other
563 stars 13 forks source link

`on()` collides with all sorts of code in the wild. #39

Closed bmeck closed 1 week ago

bmeck commented 1 year ago

Lots of libraries and runtimes use on() as a generic event handling mechanism. This name might cause problems for them updating and as such cause unnecessary friction. It would be good to see if there is another name that is less prevalent in the same design space that wouldn't collide with Cloudflare Workers, Bun, Node.js, etc. and the libraries written for them (Note: all of these have some form of EventTarget, but often mix with EventEmitter's on in the wild or for backwards compatibility).

ming-codes commented 1 year ago

Yup, that was my first reaction as well. I still like the Observable.from from the old proposal

benlesh commented 1 year ago

That's very true. I personally don't care if it's on or some other name. button.events('click'), button.when('click'), button.observe('click') or some such thing.

@ming-codes: I'm not sure what you mean by Observable.from, are you saying that EventTarget would somehow implement a symbol or something that it would recognize? Since addEventListener requires a sort of magic string like "click" or "message" or whatever, I'm not sure that's doable. (it's going seeing your name on github again, btw, it's been a minute)

ljharb commented 1 year ago

Yes, Observable.from would be like Iterator.from, which response to Symbol.iterator - iow, Symbol.observable would be the protocol that an object uses to indicate it can be observed, and Observable.from would reliably extract that.

ming-codes commented 1 year ago

are you saying that EventTarget would somehow implement a symbol or something that it would recognize?

Not my I original thought, but I like it. ๐Ÿ˜„ The Symbol could implement a function that returns an Observable.

[Symbol.observable](eventName) {
   ...
}
luijar commented 1 year ago

Completely agree. Besides less possibilities of collissions, I think a method like .observe() on the EventTarget instance is much more straightforward and clear.

domfarolino commented 11 months ago

I'd like to get a better feel for how much collision there might actually be in the wild. Are there huge libraries that supply a .on() method on objects that are currently also EventTargets? It's not clear to me how this would collide with Cloudflare workers, Node.js, etc. as mentioned in the OP.

From looking at Cloudflare workers, they indeed support EventTarget but have nothing to do with EventEmitter (and it's corresponding .on()), so the only surface area for collision here I guess is 3p library code that does what I mentioned above. Node.js implements EventTarget which doesn't provide its own .on(), so I don't see an obvious collision there, but I guess they also have NodeEventTarget which does have .on() (that seems to delegate to the EventEmitter implementation I guess?). That's interesting, but those objects aren't actual EventTargets anyways so maybe it is OK...

Anyways, some concrete examples of this being an issue would be very useful to see!

domenic commented 11 months ago

Yeah, I think in-the-wild data from browser use counters would be necessary to believe there's some sort of unsolvable conflict here.

bmeck commented 11 months ago

I would prefer non-browser data be included as well from something like WinterCG or somewhere. Anything providing both EventTarget and EventEmitter interfaces (not inheritance) would be affected.

domfarolino commented 8 months ago

At some point I can try and look into use counter data for Chromium, but since @bmeck filed this concern initially I'd love if you could take the first leap and help us track down this data from WinterCG or elsewhere, to try and substantiate the concern here.

domfarolino commented 8 months ago

One slightly interesting conflict I encountered when implementing the EventTarget integration with Observables is this WPT: https://github.com/web-platform-tests/wpt/blob/master/trusted-types/trusted-types-event-handlers.html#L36-L40. Basically this test iterates over everything in the HTMLDivElement prototype and expects anything that begins with on to be an event handler, which it would no longer be given how this proposal is currently shaped.

I have no idea how common this kind of thing is in frameworks or user code. I'd certainly hope it's not common enough to preclude us from using what I think is a nice name like on for this API, but it would be great if any framework authors could weigh in with any insight here.

triskweline commented 8 months ago

While I'm all in favor of short names, we have two decades of history where a function on() traditionally took an event type and a callback. Just off the top of my head I can remember:

Although the only hard namespace conflict is Prototype.js, choosing on() will certainly break expectations for how the API works. It will also be somewhat awkward to work with code that uses both traditional on and the new observable API introduced by this proposal.

I hope it is still possible to use a more descriptive name like observe() that comes with less historical baggage.

domfarolino commented 7 months ago

These are all really good examples, thanks a lot for providing them. They make a strong case for the theoretical problem of name collisions with new/confusing behavior under an old name (on()), however I'd really like to know how much our on() will mess things up, so here's what I'm kind of leaning towards at this point. Chromium has the Origin Trial infrastructure and I think it gives us a good opportunity to trial the Observable API and its EventTarget integration with the on() API, and use that as a concrete way to collect feedback from OT users in the wild, that are using many of the frameworks you listed above.

This should give us a bunch of real-world data as to how suitable this API name is, before we ever launch the API to stable, giving us plenty of flexibility to change things up mid-trial.

triskweline commented 7 months ago

Note I was really concerned about breaking expecations, not about actual name collisions. The only name collision from the list above is Prototype.js (which patches Element.prototype). This is a very old framework, and a 2007-era Prototype.js app is unlikely to use this new observable API.

Sorry that I couldn't make this clearer.

domfarolino commented 7 months ago

No I think that was clear. I tried to capture that in:

with new/confusing behavior under an old name (on())

but could've been clearer myself actually.

ljharb commented 7 months ago

Note that an origin trial wonโ€™t address or expose user confusion issues, since it simply wonโ€™t have anywhere near enough exposure to devs to provide that info.

domfarolino commented 7 months ago

Hmm, I'm not sure. It's caught lots of things like this in the past. To say that it simply cannot provide a useful signal is to say the entire Origin Trial infrastructure is useless, which seems self-evidently false.

ljharb commented 7 months ago

@domfarolino to me it definitely seems like it would provide runtime usage feedback, and expose a subset of devs to it, but that subset won't likely ever be representative of devs as a whole - since aren't origin trials always run on a small number of partner corporate websites?

domfarolino commented 7 months ago

Check out http://googlechrome.github.io/OriginTrials/developer-guide.html. Any site can opt-in to the Origin Trial. The maximum experiment population size is 0.5% of all page loads across Chrome.

benlesh commented 7 months ago

Although the only hard namespace conflict is Prototype.js

FWIW: This shouldn't break Prototype.js apps. Prototype will conflict with and overwrite it, but it won't break.

They're creating a new Element class from the existing global element, then they're adding methods to it by trampling whatever is there (note the lack of the third argument for mergeMethods means it will trample whatever is there).

benlesh commented 7 months ago

If we're doing all of this research on the name on, do we have alternative names that we could check at the same time?

dasa commented 6 months ago

Dojo also uses on: https://dojotoolkit.org/reference-guide/1.10/dojo/Evented.html

/cc @dylans

dasa commented 6 months ago

I found that when using Chrome 123 and setting chrome://flags/#observable-api, the Dojo example code-dialog is broken at: https://dojotoolkit.org/reference-guide/1.10/dojo/Evented.html#examples

The example itself runs fine, but when you close the floating "CodeGlass" dialog with the small X in the upper right corner, this error is shown in the console, and you can't reopen the example when you click the Run button again.

image
triskweline commented 6 months ago

If we're doing all of this research on the name on, do we have alternative names that we could check at the same time?

observe() would be right there :slightly_smiling_face:

domfarolino commented 6 months ago

observe() sounds nice. However I think there was some discussion at TPAC 2023 about making other "observer"-like things โ€” like MutationObserver and IntersectionObserver โ€” into Observables in the fullness of time. I recall @smaug---- mentioning something like this.... maybe. In that case, observe() would be tricky, because of its observe() method.

dasa commented 5 months ago

If we're doing all of this research on the name on, do we have alternative names that we could check at the same time?

How about something more explicit like createObservable?

domfarolino commented 5 months ago

I think observe() is probably the best, and I think it could probably work even in spite of my prior concerns, since the Observer#observe() method should be able to be disambiguated with MutationObserver#observe() and IntersectionObserver#observe().

triskweline commented 5 months ago

since the Observer#observe() method should be able to be disambiguated with MutationObserver#observe() and IntersectionObserver#observe().

They also won't share a common interface as these methods require different arguments. E.g. observing an Element requires an event type, but observing a MutationObserver requires either no arguments (if we're observing all registered elements) or an Element (if we're only observing a single element registration).

smaug---- commented 5 months ago

MutationObserver.observe() certainly requires at least one argument.

dasa commented 4 months ago

Is there consensus to rename on to observe?

domfarolino commented 4 months ago

I think I am fine with it, but only if we actually need to change it from on()

dasa commented 4 months ago

I did show above how on breaks the Dojo 1.x API.

At https://github.com/WICG/observable/issues/39#issuecomment-1870648887 you mentioned running an Origin Trial for this. Is that still the plan?

domfarolino commented 4 months ago

I think we'll run an Origin Trial before shipping probably.

pheede commented 4 months ago

@domfarolino I will chime in and reinforce @dasa's point from above: there is a real-world collision in the Dojo 1.x API and on very much needs to be renamed (to observe or otherwise) to avoid widespread impact.

Dojo 1.x is used - among other places - in a series of widely deployed products that @dasa and I represent. The consequence of shipping as-is would be significant breakage for likely tens of thousands of our customers that would need to aggressively patch or upgrade systems at great cost to them and with significant end-user impact. Happy to provide additional context or insight if needed.

dylans commented 4 months ago

We tried really hard with Dojo 1.x to not pollute the global namespace, but unfortunately we didn't consider how this type of scenario would work.

Also I'm all for short names for commonly used things, but in experience, having an API called on was not great in terms of documentation discoverability.

domfarolino commented 3 months ago

I was looking more at the Dojo breakage that @dasa kindly pointed out in https://github.com/WICG/observable/issues/39#issuecomment-1933132797 and I want to make sure I understand why the existence of EventTarget#on() breaks things. It's especially curious that the example actually runs fine, but closing the demo window is what blows up.

Can someone familiar with this clarify what mechanism exactly is causing the breakage?

dasa commented 3 months ago

@dylans could probably explain this better, but this is where on is defined: https://github.com/dojo/dojo/blob/master/Evented.js#L24

And this is where it's called: https://github.com/dojo/dojo/blob/master/on.js#L73

domfarolino commented 3 months ago

Yeah, unfortunately that doesn't give me a lot to go off of! I would love to understand how the mere existence of EventTarget#on is expected to break Dojo 1.x. And also, is it expected to break Dojo 2.x at all? My understanding is that people are transitioning to it from 1.x maybe, from reading the docs?

dasa commented 3 months ago

I would love to understand how the mere existence of EventTarget#on is expected to break Dojo 1.x.

I've created a test case at https://jsbin.com/wevafawici/1/edit?html,console

When dojo/on finds and calls the target's on method, it expects to be calling the on method defined in dojo/Evented. When the Observable API is enabled, it get's an observable instead, and in that test case, the handler for the window's "load" event is never called.

And also, is it expected to break Dojo 2.x at all? My understanding is that people are transitioning to it from 1.x maybe, from reading the docs?

I'm less familiar with Dojo 2, which is known as the Dojo Framework and requires a rewrite of apps.

bakkot commented 3 months ago

Specifically, this bit:

if(typeof target.on == "function" && typeof type != "function" && !target.nodeType){
    // delegate to the target's on() method, so it can handle it's own listening if it wants (unless it
    // is DOM node and we may be dealing with jQuery or Prototype's incompatible addition to the
    // Element prototype
    return target.on(type, listener);
}
jasnell commented 2 months ago

Just weighing in on this, I think the suggestion to have eventTarget.observe('foo') instead of eventTarget.on('foo') works rather nicely.

domfarolino commented 2 months ago

I agree that's probably the right fallback name, if we have to go that route. However, I do want to try one more trick that I discussed with @domenic recently โ€” making the EventTarget#on() method behave just like native addEventListener() if the second argument is a function instead of an ObservableEventListenerOptions dictionary.

I'm hopeful that will trigger the codepath @bakkot pointed out in https://github.com/WICG/observable/issues/39#issuecomment-2103327016, and continue to fire things correctly and unbreak almost all of the breakage/conflict we're seeing here with Dojo. It's a bit ugly, but it could allow us to keep the on() method in this proposal which I think people are pretty excited about!

jasnell commented 2 months ago

Overloading the on(...) in that way would still conflict with some cases. For instance, Node.js has a special subclass of EventTarget (https://nodejs.org/docs/latest/api/events.html#nodeeventtargetontype-listener) that allows it largely to also be used as a Node.js EventEmitter. The class has an on(...) method that returns this (that is, the NodeEventTarget instance) rather than an Observable. It would be rather awkward and confusing for users for that method to suddenly end up with a non-standard polymorphic return value.

Given that we know for certain that on(...) is used in this space already with very specific semantics, renaming is likely the much better choice here.

dasa commented 2 months ago

I think that would still break Dojo since when it call's on, it expects an object with a remove property to be returned. This is shown in the stack trace in my first error report, where it shows "e.remove is not a function".

domfarolino commented 2 months ago

Alrightly, it sounds like the conflict with dojo & kin may very well be fatal for the on() method proposed here, so I think we have no choice but to rename it ๐Ÿ˜”. Thank you for all of the discussion here though, I appreciate helping us explore alternatives.

I think we landed on observe() as the best name moving forward, however I do have one concern with it. I think it could be confusing since observe() doesn't actually do anything or observe anything. It just returns an Observable that lets you observe things. This is in contrast with other observe() methods, like IntersectionObserver#observe() and MutationObserver#observe(), which are more "active". This distinction is especially important if we consider future integrations between Observables and these other APIs (see https://github.com/WICG/observable/issues/72#issuecomment-2035327296), where an observe() overload on these APIs might "lazy" return Observables instead of doing something more active.

I think @annevk will likely be interested in the discussion here, as he is helping form a stance on this API for WebKit, and the name we choose is a big deal.

dasa commented 2 months ago

I think we landed on observe() as the best name moving forward, however I do have one concern with it. I think it could be confusing since observe() doesn't actually do anything or observe anything. It just returns an Observable that lets you observe things.

Maybe createObservable? This would be similar to a lot of other createX methods like createDataChannel and createIndex etc.

domfarolino commented 1 month ago

Eh, createObservable() feels a little verbose and lengthy to me. One upside with on() was how incredibly short and sweet is. I've been coming around to when() honestly.

ljharb commented 1 month ago

when seems like a great choice, like then but can happen 0 or N times :-)

PowerKiKi commented 1 month ago

There is jQuery.when(). Could this have a similar issue than Dojo's on() ?

triskweline commented 1 month ago

There is jQuery.when(). Could this have a similar issue than Dojo's on() ?

jQuery does not patch Element.protoype, but requires elements to be wrapped in a jQuery collection to use its API.

In addition, jQuery.when() is defined on the global jQuery object, not on jQuery element collections (which would be jQuery.fn.when()).

domfarolino commented 1 month ago

upon() is another idea I've heard and think is a good candidate too. However I think when() is just a touch nicer. I will submit a PR soon to rename on() to when(), just FYI!

Thank you for the discussion here.