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

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

Closed
bmeck opened this issue Jul 31, 2023 · 61 comments · Fixed by #161
Closed

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

bmeck opened this issue Jul 31, 2023 · 61 comments · Fixed by #161
Assignees

Comments

@bmeck
Copy link

bmeck commented Jul 31, 2023

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
Copy link

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

@benlesh
Copy link
Collaborator

benlesh commented Aug 11, 2023

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
Copy link

ljharb commented Aug 11, 2023

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
Copy link

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
Copy link

luijar commented Aug 21, 2023

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

@domfarolino
Copy link
Collaborator

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
Copy link
Collaborator

domenic commented Aug 30, 2023

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
Copy link
Author

bmeck commented Aug 30, 2023

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link

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
Copy link
Collaborator

domfarolino commented Dec 27, 2023

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
Copy link

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
Copy link
Collaborator

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
Copy link

ljharb commented Dec 29, 2023

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
Copy link
Collaborator

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
Copy link

ljharb commented Dec 30, 2023

@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
Copy link
Collaborator

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
Copy link
Collaborator

benlesh commented Jan 4, 2024

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
Copy link
Collaborator

benlesh commented Jan 4, 2024

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
Copy link

dasa commented Feb 7, 2024

@dasa
Copy link

dasa commented Feb 7, 2024

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
Copy link

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 🙂

@domfarolino
Copy link
Collaborator

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
Copy link

dasa commented Feb 26, 2024

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
Copy link
Collaborator

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
Copy link

triskweline commented Feb 27, 2024

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).

@domfarolino
Copy link
Collaborator

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
Copy link

ljharb commented Jun 25, 2024

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

@PowerKiKi
Copy link

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

@triskweline
Copy link

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
Copy link
Collaborator

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.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jul 22, 2024
See the discussion in WICG/observable#39,
which led to this decision.

R=jarhar

Bug: 40282760
Change-Id: I9cd744460a3545c74fb34a0418c6ffddc5e6d4e1
@domfarolino
Copy link
Collaborator

Update, I've put an X poll out for this: https://x.com/domfarolino/status/1815458583334842691 to get a feel for how the community feels.

@benlesh
Copy link
Collaborator

benlesh commented Jul 22, 2024

I'm partial to when. I don't like verbs like listen or observe because they sound like they start the subscription, although observe isn't that bad.

I'd consider how it reads in composition like:

element.when('mousedown')
  .switchMap(start => document.when('mousemove')
    .map(current => [
      current.clientX - start.clientX,
      current.clientY - start.clientY,
    ])
    .takeUntil(document.when('mouseup'))
  .subscribe()

@tetsuharuohzeki
Copy link
Contributor

Update, I've put an X poll out for this: https://x.com/domfarolino/status/1815458583334842691 to get a feel for how the community feels.

I would like to vote .listen() in that reply.

@Jamesernator
Copy link
Contributor

I don't like verbs like listen or observe because they sound like they start the subscription

I have the same feeling, but I feel like a noun like .events(...) or similar makes more sense.

@benlesh
Copy link
Collaborator

benlesh commented Jul 23, 2024

I'd still push on on, and keep when in our back pocket though.

@domfarolino
Copy link
Collaborator

Sadly I think we have to call time of death on on() — I don't think the conflict is something we can overcome.

In the poll .observe() is winning, but I'm sympathetic to the concern of using a verb (like .observe()) which sounds very active, even though it just returns a "lazy" Observable.

Now I'm trying to see if the thoughts in #72 will influence our decision at all. If we give *Observer APIs the ability to vend Observables in the future via an overloaded .observe() method, then does that mean we should also go with .observe() for EventTarget? Or is EventTarget special for some reason.

@domenic
Copy link
Collaborator

domenic commented Jul 24, 2024

Now I'm trying to see if the thoughts in #72 will influence our decision at all. If we give *Observer APIs the ability to vend Observables in the future via an overloaded .observe() method, then does that mean we should also go with .observe() for EventTarget? Or is EventTarget special for some reason.

I love the holistic thinking here.

Let's make it a bit more concrete. If we used observe() for both with the static method option from #72 then we'd have:

// (1)
imageElement.observe("load").subscribe(...);
ResizeObserver.observe(imageElement).subscribe(...);

If we used observe() for EventTarget with the instance method version from #72:

// (2)
imageElement.observe("load").subscribe(...);
imageElement.observeResizes().subscribe(...);

If we use when() for EventTarget and observe() for the static method option from #72 then we'd have:

// (3)
imageElement.when("load").subscribe(...);
ResizeObserver.observe(imageElement).subscribe(...);

If we use when() for EventTarget and the instance method version from #72:

// (4)
imageElement.when("load").subscribe(...);
imageElement.whenResized().subscribe(...);

Overall I like (2) and (4) the most. It feels like a great unification of events and XObservers, where the XObservers just need slightly more specialized methods instead of the generic event-name-taking method. This also ties into the discussion in #72 about how maybe these methods might need to return subclasses.

(1) is also reasonable, although it might be a bit confusing that the arguments to observe() are so different.

I think (3) feels bad, but concretely wouldn't be the worst thing. It just makes the APIs feel completely separate. Which, to be fair, they kind of are; they just share a return type.

@jasnell
Copy link

jasnell commented Jul 24, 2024

Of all the alternatives discussed, I'm +1 for when or observe. Of the options presented by @domenic, I'm partial to (2) and (4) also, tho I do also like the symmetry of (1).

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jul 26, 2024
See the discussion in WICG/observable#39,
which led to this decision, and
WICG/observable#161 for the corresponding spec
rename.

R=jarhar

Bug: 40282760
Change-Id: I9cd744460a3545c74fb34a0418c6ffddc5e6d4e1
@domfarolino
Copy link
Collaborator

domfarolino commented Jul 26, 2024

I thought about this a bit more, and I think the visual symmetry of (1) is misleading. More generally, I think the symmetry of having a pre-existingXObserver#observe() instance method and either EventTarget#observe() instance method or XObserver#observe() static method is misleading.

It means:

  1. Both EventTarget and XObserver have instance methods that share a name but have completely different behavior (one is passive and the other is active)
  2. XObserver itself has two "different" methods with the same name: one static and passive, one instance and active. But both by the same name!

That worries me, and pushes me to option (3) or (4) — when(). Option (4) looks the cleanest, but I'm trying to think through how much of a limitation it is that Element#whenResized() produces an RO Observable only for the element it's called on.

Let's keep going with the ResizeObserver-specific example. Today, when you use ResizeObserver, you create one with a single potentially massive and complicated callback, and then you can call observe() any number of times with different target elements. The same shared callback is used for processing all resizes, for all elements being observed, and presumably there is some (performance?) value to that. But with Element#whenResized() and no work done to ResizeObserver itself, you have to call el.whenResized().subscribe(massiveComplicatedCallback) on many elements, and have potentially many copies of that callback floating around, with many different internal ResizeObservers under the hood. That's a lot less shared infrastructure, but maybe that's OK.

It was out of hope for "sharing more stuff" that I mentioned an Observable-returning method on XObserver that's not static. I envisioned something like new ResizeObserver(massiveCallback).whenResized(element).subscribe(), but hadn't thought it through — I don't think this works at all since the callback would need access to subscriber to plumb values downstream. So I think the only way to do it would be to add a static method on XObserver which is option (3), and doesn't seem all that useful. It takes me to @domenic's comment in #72 (comment):

An XObserver with no callback is basically storing no state so it doesn't really make sense to use instance methods.

... an argument I might extend, to apply to static methods too. That is, ResizeObserver.whenResized().subscribe(massiveCallback) doesn't seem any more useful than element.whenResized().subscribe(massiveCallback). I guess the only benefit is that shares the same internal ResizeObserver under the hood. (That, and for MutationObserver we get to keep takeRecords() where it is.)

@domfarolino
Copy link
Collaborator

All of that is to say that at least on this thread, I think I feel settled on when(). I think we need to spend more time thinking about integration with other APIs in #72, but I think we have enough to go off of here to make the decision on when() in the meantime, regardless of how the integration details look. Thoughts?

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jul 29, 2024
See the discussion in WICG/observable#39,
which led to this decision, and
WICG/observable#161 for the corresponding spec
rename.

R=jarhar

Bug: 40282760
Change-Id: I9cd744460a3545c74fb34a0418c6ffddc5e6d4e1
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Aug 13, 2024
See the discussion in WICG/observable#39,
which led to this decision, and
WICG/observable#161 for the corresponding spec
rename.

R=jarhar

Bug: 40282760
Change-Id: I9cd744460a3545c74fb34a0418c6ffddc5e6d4e1
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Aug 13, 2024
See the discussion in WICG/observable#39,
which led to this decision, and
WICG/observable#161 for the corresponding spec
rename.

R=jarhar

Bug: 40282760
Change-Id: I9cd744460a3545c74fb34a0418c6ffddc5e6d4e1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5730072
Reviewed-by: Joey Arhar <jarhar@chromium.org>
Commit-Queue: Dominic Farolino <dom@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1340875}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Aug 13, 2024
See the discussion in WICG/observable#39,
which led to this decision, and
WICG/observable#161 for the corresponding spec
rename.

R=jarhar

Bug: 40282760
Change-Id: I9cd744460a3545c74fb34a0418c6ffddc5e6d4e1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5730072
Reviewed-by: Joey Arhar <jarhar@chromium.org>
Commit-Queue: Dominic Farolino <dom@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1340875}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Aug 15, 2024
…get#when(), a=testonly

Automatic update from web-platform-tests
DOM: Rename EventTarget#on() to EventTarget#when()

See the discussion in WICG/observable#39,
which led to this decision, and
WICG/observable#161 for the corresponding spec
rename.

R=jarhar

Bug: 40282760
Change-Id: I9cd744460a3545c74fb34a0418c6ffddc5e6d4e1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5730072
Reviewed-by: Joey Arhar <jarhar@chromium.org>
Commit-Queue: Dominic Farolino <dom@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1340875}

--

wpt-commits: c4e3d193ad0d0e708ad2f252a245c368df098c6e
wpt-pr: 47237
ErichDonGubler pushed a commit to erichdongubler-mozilla/firefox that referenced this issue Aug 19, 2024
…get#when(), a=testonly

Automatic update from web-platform-tests
DOM: Rename EventTarget#on() to EventTarget#when()

See the discussion in WICG/observable#39,
which led to this decision, and
WICG/observable#161 for the corresponding spec
rename.

R=jarhar

Bug: 40282760
Change-Id: I9cd744460a3545c74fb34a0418c6ffddc5e6d4e1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5730072
Reviewed-by: Joey Arhar <jarhar@chromium.org>
Commit-Queue: Dominic Farolino <dom@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1340875}

--

wpt-commits: c4e3d193ad0d0e708ad2f252a245c368df098c6e
wpt-pr: 47237
@triskweline
Copy link

Thank you for considering these concerns ❤️

gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Aug 23, 2024
…get#when(), a=testonly

Automatic update from web-platform-tests
DOM: Rename EventTarget#on() to EventTarget#when()

See the discussion in WICG/observable#39,
which led to this decision, and
WICG/observable#161 for the corresponding spec
rename.

R=jarhar

Bug: 40282760
Change-Id: I9cd744460a3545c74fb34a0418c6ffddc5e6d4e1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5730072
Reviewed-by: Joey Arhar <jarharchromium.org>
Commit-Queue: Dominic Farolino <domchromium.org>
Cr-Commit-Position: refs/heads/main{#1340875}

--

wpt-commits: c4e3d193ad0d0e708ad2f252a245c368df098c6e
wpt-pr: 47237

UltraBlame original commit: 635103775408664b51f70080db91b67ba60941b8
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Aug 23, 2024
…get#when(), a=testonly

Automatic update from web-platform-tests
DOM: Rename EventTarget#on() to EventTarget#when()

See the discussion in WICG/observable#39,
which led to this decision, and
WICG/observable#161 for the corresponding spec
rename.

R=jarhar

Bug: 40282760
Change-Id: I9cd744460a3545c74fb34a0418c6ffddc5e6d4e1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5730072
Reviewed-by: Joey Arhar <jarharchromium.org>
Commit-Queue: Dominic Farolino <domchromium.org>
Cr-Commit-Position: refs/heads/main{#1340875}

--

wpt-commits: c4e3d193ad0d0e708ad2f252a245c368df098c6e
wpt-pr: 47237

UltraBlame original commit: 635103775408664b51f70080db91b67ba60941b8
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 a pull request may close this issue.