removing event delegates in favor of onevent props

[ryhinchey]

This (draft) PR removes assigning and removing event handlers to delegates (child dom elements). My proposal is for Crank to use onevent props for all global event handlers emitted from dom elements. By removing delegates, we can reduce complexity in Crank, make the mental model for how events work easier to understand, and remove a potentially expensive process of looping through child elements to add/remove listeners.

Related discussion around onevent props: #10,

This would leave the methods inherited from CrankEventTarget to be used for other, equally important scenarios. The power of having an event emitter that limits event bubbling to parent components is super powerful (as compared to a global event emitter like what's in node). In this way, CrankEventTarget gives the power of a beautiful redux-like experience built into the framework (and a lot more).

Related discussion: #15

Todo:

• tests
• update documentation & website
• update examples

I'd love to see these suggestions implemented in Crank and it seems there's a good number of folks who feel the same way. Thanks for reading and I'm looking forward to getting feedback on this PR!

[ryhinchey]

Also - while not officially a part of this proposal, if this were to land it would be straightforward to replace the event-target-shim dependency with our own lightweight version (or a tiny dependency like mitt).

event-target-shim is ~2kb minified and gzipped and we wouldn't need most of what it provides.

[brainkim]

Ehhhh... I’m okay with people using onevent props in their own code, but as the first user of Crank I’m really hesitant to let event delegation go. Also, event delegation is more flexible in a lot of ways. For instance, how do you attach events on children which have been passed in:

function *Router() {
  this.addEventListener("click" , (ev) => {
    if (ev.target.tagName === "A") {
      ev.preventDefault();
      // do some stuff
    }
  });

  for (const {children} of this) {
    yield children;
  }
}

At the end of the day, this.addEventListener is how I’d like to listen to child elements, and I think JSX without the clutter of callback props is actually quite beautiful. If I’m going to compromise and let people do callback props maybe y’all can compromise and let me, the creator, use event targets? Haha.

[brainkim]

Another thought: the current state of EventTarget implementations is kinda sad, indicating we’ll have to use a custom one. Safari and JSDOM event targets fail silently when trying to dispatch events, and there is no notion of parents/children/bubbling with the custom EventTarget class.

[Drache93]

Just a side note - current way is great with RxJS.

function* RandomDogApp() {
  let throttle = false;

  fromEvent(this, "click")
    .pipe(filter(ev => ev.target.tagName === "BUTTON"))
    .subscribe(() => {
      throttle = !throttle;
      this.refresh();
    });

  while (true) {
    yield (
      <Fragment>
        <div>
          <button>Show me another dog.</button>
        </div>
        <RandomDogLoader throttle={throttle} />
      </Fragment>
    );
  }
}

[ryhinchey]

@Drache93 that is super clean! Nice

[ryhinchey]

@brainkim here are some responses and again -thanks for creating this awesome framework. I'm excited about the direction this is going and am only making suggestions as someone who wants this to succeed.

but as the first user of Crank I’m really hesitant to let event delegation go

completely fair which is why I made this a draft PR 😄 . I appreciate the discussion happening and will eventually close this PR.

For instance, how do you attach events on children which have been passed in:

Your suggestion works and is a small amount of code which is great. However, I wouldn't want to listen to every <a> tag rendered under my Router. In general, I try to reduce excess event noise as much as possible and get as granular as I can about which elements I'm attaching listeners to. I'd probably look to do something like this (given it will only loop over direct children):

function* Router() {
  const onclick = ev => {
    ev.preventDefault();
  };
  
  const addListenerToChildren = children => {
    return children.map(child => {
      if (child.tag === "a") {
        return createElement(child.tag, { ...child.props, onclick });
      }

      return child;
    });
  };
  
  for (const { children } of this) {
    yield addListenerToChildren(children);
  }
}

Another option I'd consider would be using Crank's version of context: this.set and this.get.

I actually have a react-router inspired router for Crank here. Still a lot of work to be done but check it out and let me know what you think!

If I’m going to compromise and let people do callback props maybe y’all can compromise and let me, the creator, use event targets?

100%. I'm glad that callback props made their way to the docs and are accepted into the framework. There really isn't a strong reason why Crank shouldn't allow both, especially at this early stage.

the current state of EventTarget implementations is kinda sad, indicating we’ll have to use a custom one. Safari and JSDOM event targets fail silently when trying to dispatch events, and there is no notion of parents/children/bubbling with the custom EventTarget class.

Agreed there is work to be done and this is somewhat related to the issue I opened #46. The fact that we're using a custom event system to listen to native events from dom elements like buttons and Crank components is problematic. Instead of using the an event target's native dispatchEvent, addEventListener, and removeEventListener, we need to have our own system so dom elements like buttons and Crank components can communicate over the same event system. When I mention reducing complexity in Crank, this is part of what I'm thinking about. Perhaps I'm missing something here and it's simpler than I perceive it though.

[brainkim]

@ryhinchey
I guess this is just a difference of opinion. I look at both of your arguments and came to the opposite conclusion somehow 😄

However, I wouldn't want to listen to every tag rendered under my Router. In general, I try to reduce excess event noise as much as possible and get as granular as I can about which elements I'm attaching listeners to.

Your example wouldn’t work because we abide by the JSX semantics of only wrapping children in an array if there are multiple:

const el = <div><span /><span /></div>;
console.log(el.props.children); // [{tag: "span"}, {tag: "'span"}]
const el1 = <div><span /></div>;
console.log(el1.props.children); // {tag: "span"}

Typically, and I can’t stop people from doing otherwise, I think people should treat the children prop as a black box, only to be transcluded into your produced trees. The fact is, it can be an element or a nested iterable of elements, and this has consequences for the actual diffing algorithm so messing with the elements or flattening the items to an array can produce surprising results. Not attempting to read or manipulate children in your components makes component hierarchies much more flexible and easy to reason about.

Also, I’m not sure why doing a little DOM filtering on events in a single callback is “more noise” than manually passing the callback to each individual element. I just don’t understand why the former is noisy and the latter isn’t, and I think it has more to do with aesthetics than anything objective.

The fact that we're using a custom event system to listen to native events from dom elements like buttons and Crank components is problematic.

I’m not sure how this is problematic. The EventTarget interface provided by Crank and the DOM EventTarget system, which is the 90% use-case for Crank, implement the exact same, battle-tested interface. The goal is to have them work exactly the same, and the best way to do this is to simply mimic the APIs.

I know that EventTargets are not perfect, but it’s what’s used in the browser, and one way to think of it is this: Crank has a pragmatic approach to JavaScript and DOM features. Even if EventTargets and generators and promises are not perfect, and you’d much rather be using Rust futures or monads or something, they’re what we got as JavaScript devs, and Crank tries to do as much with what we got, in terms of APIs and runtime, as we possibly can. And trust me, without me singling out an event system, most of the event-emitter libraries available in user space are bad. Like really bad. I think doing something like using one of the myriad event-emitter systems is more cognitive load than using an event system which looks and behaves exactly like the one provided by the DOM.

And you still haven’t addressed some of the other reasons I’ve given for adding the EventTarget interface: How do we register passive listeners? How do we do event capturing? This isn’t available via the DOM 0 event prop API.

[ryhinchey]

Also, I’m not sure why doing a little DOM filtering on events in a single callback is “more noise” than manually passing the callback to each individual element. I just don’t understand why the former is noisy and the latter isn’t, and I think it has more to do with aesthetics than anything objective.

the dom filtering code isn't the "more noise" part. the "more noise" part is that I'm now listening to events fired from elements that I don't necessarily care about. At least when I pass callbacks to elements, I know that callback will only get called when that element emits an event. It's not a big deal and the mechanisms exist within Crank for me to do what I want 😄

Crank has a pragmatic approach to JavaScript and DOM features. Even if EventTargets and generators and promises are not perfect, and you’d much rather be using Rust futures or monads or something, they’re what we got as JavaScript devs, and Crank tries to do as much with what we got, in terms of APIs and runtime, as we possibly can

I can fully get behind this.

And you still haven’t addressed some of the other reasons I’ve given for adding the EventTarget interface: How do we register passive listeners? How do we do event capturing? This isn’t available via the DOM 0 event prop API.

This PR doesn't remove the event target api from Crank and I wouldn't suggest that we do that. We want passive listeners and event capturing. Using the DOM 0 api, you're knowingly sacrificing those things for a simpler syntax. Having both options is super powerful.

I'll close this PR as it's clear removing event delegates isn't going to happen now. I appreciate the time and thought that went into this discussion!