bikeshaving / crank

The Just JavaScript Framework
https://crank.js.org
MIT License
2.7k stars 75 forks source link

addEventListener and delegation with tagName is awkward #10

Closed tomhicks closed 4 years ago

tomhicks commented 4 years ago

Not an issue per se (like a bug or anything), but having just read through all the blog post and docs, this was the one bit that I thought "modern React" definitely does better at.

No idea if this is even possible, but I'm sure I won't be the only one turned off by this.

brainkim commented 4 years ago

Thank you for the feedback and congrats on being the first issue in this repository. I anticipated this concern (why use this.addEventListener("click", callback) rather than onClick={callback}?) and have a coked out journal entry describing reasons for this design decision which I will try to distill in this issue.

I didn’t want to follow the onClick callback API because over the years I‘ve found it lacking in several key respects:

  1. onClick-APIs are a leaky abstraction.

EventTargets in the DOM take more than just a function; they take several options including once, capture and passive. React has attempted to model capture with onClickCapture-type props, and once event listeners can be emulated with state, but there is still currently no solution for registering passive event listeners, and you can sorta see the folly of using props, which are singular values, to model EventTarget listeners (onHoverCapturePassive). I guess you could make the onClick-type props take a tuple, but that seems immediately more unwieldy that the API Crank proposes. In fact, there has been some work in React trying to support more of the EventTarget API with hooks to address these concerns.

  1. onClick-APIs make JSX look terrible.

I know that you can assign callbacks to a variable and pass them into JSX by reference, but so long as it’s possible to write event listeners inline, people will write them inline. And when you do, you lose a lot of the similarities JSX has with HTML, because functions are multiline syntax structures and don’t map to anything you can do in HTML. IMO, inline functions in JSX are just plain ugly.

  1. onClick-APIs force you to dox anonymous event listeners.

Even when you define your callbacks as variables before passing it into JSX, it can be somewhat taxing to have to come up with names for these callbacks. When I’ve used React, I would typically use conventions like this.handleClick or const handleModalClick = for callbacks which would be passed to child components, but I still felt untethered. Using the this.addEventListener API removes this specific pain point, by allowing callbacks to be understood by usage rather than named.

  1. Mimicking onClick-APIs is a pain when writing components.

Often with React, you would create components with onWhatever properties to match the host element event props. This was always a pain because event callbacks are almost always an optional part of component APIs, so you would either have to do checks to make sure you weren’t invoking nullish values, or pass in a dummy noop handler.

And even if you did this, the question of what to call your callback props with was a problem. People pass arbitrary things to event callbacks in React: I’ve seen virtually every type of value used except, ironically, for Event objects, which is what host elements use. Why do component callbacks pass arbitrary values while host callbacks pass events? By using event bubbling and the CustomEvent constructor, we can actually make Component events seem like regular DOM events. The fact that you bubble events with this.dispatchEvent also means that you can actually see if the parent calls ev.preventDefault(), giving you even greater expressiveness when handling event dispatch, and a better pattern for controlled/uncontrolled components.

I think having this.dispatchEvent and custom events is incredibly powerful, and I think it even obsoletes Redux/state-mgmt solutions, in the sense that you can define all the custom events of your app in one root component. It‘s even mildly type-checkable, if you look at the TODOMVC-ts example.

  1. Passing event handlers between multiple layers of components is a pain in the butt.

Have you ever had a parent component which needed to listen to an event on a component it didn’t own? This often meant having to pass callbacks between successive layers of components, each time coming up with names for what the props should be called and solving the problems in 4. at each layer. You could use contexts, which this library does have a version of, but why bother? Just use event delegation and this problem is solved. Also, when components are passed children from their parents, it’s impossible with onClick-APIs to actually listen to events on the passed in children. Again, another point in favor of using event delegation. In short, this.addEventListener makes your component hierarchies more flexible.


Ultimately, I agree with you that the this.addEventListener API can feel a little foreign, and that filtering by tagName is a pain, especially because in TypeScript, ev.target/currentTarget are kinda borked right now. I dunno, lemme know if you come up with better alternatives, but I am currently really happy with this.addEventListener.

apieceofbart commented 4 years ago

Thanks a lot for this explanation - I had the same question as the author of the issue so perhaps linking this issue in docs is a good idea as more people will have similar thoughts (somewhere under "Dispatching events" section here https://crank.js.org/guides/components)

tomhicks commented 4 years ago

Thanks for the detailed response. I accept some bits of your argument.

I'm not convinced by the "you lose some inherent DOM functionality" argument. The entire Virtual DOM 'revolution' is built on the idea that for building most applications, you don't want the full DOM API. Like, you can't use appendChild and friends (you could but those elements might not last long).

The smaller API for event handling brings more structure, and "prop-drilling" is, in my opinion, one of the good parts of React. If you're passing something through things that don't need it, there's often another construct that models your situation better (it's not necessarily Context API).

From a PR perspective, I think a better example in the docs than hanging events together by string names would be good.


I suppose Crank is like React but a) uses more existing JS runtime bits, and b) uses more DOM-like patterns.

I think a) is great. I'm not so sure about b), personally. That feels like one of the great bits of React which got a lot of people into it in the first place (Components and their APIs are considered more important than the DOM) whereas this seems to do away with a really great part of having strong Component API definitions.

AlexGalays commented 4 years ago

If prop drilling is a problem for events, then it's also a problem for data?

brainkim commented 4 years ago

@tomhicks

From a PR perspective, I think a better example in the docs than hanging events together by string names would be good.

I agree. I think something like a quick Todo app like React does on its homepage would be good.

I'm not convinced by the "you lose some inherent DOM functionality" argument... That feels like one of the great bits of React which got a lot of people into it in the first place (Components and their APIs are considered more important than the DOM)

Hmmm I see where you’re coming from but lemme provide this counterpoint. The EventTarget interface is specced in the DOM Living Standard, which states in its abstract:

DOM defines a platform-neutral model for events, aborting activities, and node trees.

We can’t say that EventTargets have been used or are even popular in any environment outside the DOM, and even Safari today still doesn’t implement custom EventTargets, but I can say this about them: When it comes to JavaScript event classes, the EventTarget class is the best specified, most battle-tested, and most flexible of all of them. Think of how many event emitter classes there are on NPM right now. And I guarantee half of them don’t have concepts like passive event listeners, event capturing and bubbling, or event cancellation. In short, it is the most complete solution out there and insofar as Crank is a UI framework, I thought it was best to follow in its footsteps, especially because it mapped so transparently to the DOM use-case.

So if you’re balking because you think event targets are too “DOM”-y, I understand, but I also think that they’re the future, and hope to see the class used in more non-DOM contexts as well.

brainkim commented 4 years ago

@AlexGalays

If prop drilling is a problem for events, then it's also a problem for data?

Yes I think so! Which is why Crank provides a (yet to be documented) equivalent to the React context API: https://github.com/bikeshaving/crank/issues/12

tomhicks commented 4 years ago

I guess I would question whether an EventEmitter model is the right primitive with which to be building the majority of components (and by extension [composition?!], the majority of UIs).

It's massively flexible, sure, but for me, passing in handlers rather than subscribing to listeners is one of the most important parts of React and one of the reasons it became so popular.

I really think it would be a great disadvantage to lose it.

Or is it possible to keep the handler prop pattern as well?

dwelle commented 4 years ago

It's massively flexible, sure, but for me, passing in handlers rather than subscribing to listeners is one of the most important parts of React and one of the reasons it became so popular.

Carve those words into stone, brother.

I think that the prop event handlers (or whatever they're called) are one of the least controversial things about JSX today, and in fact one of its best advantages. Having to manually add & delegate event listeners moves us back to HTML-script templates.

If nothing else, I'd support both idioms.

brainkim commented 4 years ago

Okay I think I see where this is going. I get that people like the React events API, and I know that if I close this as a wontfix the library is simple enough that someone is gonna write “Drank - Crank with events.” So I’m gonna let you in on a little secret: currently, the host element code is kinda simple, and you can assign to a lot of DOM properties via element props. These properties include the onwhatever event properties, and I’m pretty sure that because this just reassigns the event listener every render, it should just work and shouldn’t need to be garbage collected.

/** @jsx createElement */
import { createElement } from "@bikeshaving/crank";
import { renderer } from "@bikeshaving/crank/dom";

function Greeting({ name = "World" }) {
  const handleClick = ev => {
    console.log(ev.target.outerHTML);
  };

  return <div onclick={handleClick}>Hello {name}</div>;
}

renderer.render(<Greeting />, document.body);

https://codesandbox.io/s/event-props-in-crank-8cg2v?file=/src/index.js:0-349

Is this satisfying enough? 😄

tomhicks commented 4 years ago

Lovely stuff.

Not to labour the point (as you've surrendered now 😃) but this API:

<User onFollow={handleFollow} />

is, I would argue, objectively better than this:

// I hope it's "follow" and not "Follow" or "followUser" or "user.folllow" or...
this.addEventListener("follow", handleFollow);
<User />

I'm now wondering whether we were talking cross-purposes and you always intended to support the former for custom components, and were only talking about DOM elements using the EventEmitter style?

tomhicks commented 4 years ago

If I misunderstood, I apologise.

Also, don't want to sound like an open source grump! This is awesome and really great to see someone who has the same objections to the way React is heading as I have been harbouring!

brainkim commented 4 years ago

I'm now wondering whether we were talking cross-purposes and you always intended to support the former for custom components, and were only talking about DOM elements using the EventEmitter style?

Listen. I can’t stop you from writing components with function props, even though I dislike them. I can’t stop you from writing components with render props for children, even though I hate that pattern with the intensity of a thousand suns. I guess I could start throwing errors or writing crazy TypeScript types to try and stop you, but that would be incredibly petty, and we’re all (probably) adults, and I need to leave open the possibility that I can be wrong about this stuff, and you might have some cool component ideas that involve event callback functions or even render props. Sometimes, you gotta pick and choose your battles, and if you think Crank is worth checking out if you can use this style of API, I am very happy to have you.

But please, consider this.dispatchEvent and custom events at some point. They can actually be strongly typed, and I promise you there’s an interesting part to this idea that you might not see yet.

😄✨

tomhicks commented 4 years ago

Yeah, you obviously have some experience that tells you that the EventEmitter pattern is preferable in most (all?) cases, and it's clearly not down to ignorance of the alternatives! 😃

Anyway, good on you for making this!

brainkim commented 4 years ago

https://codesandbox.io/s/tic-tac-toe-with-crank-r8zm3 Okay. I just came across this example using state machines and I see the pain points of not using callback props, especially in map functions where you already have everything you need for the callback directly in scope. I’m on board and I’m sorry.

nicosommi commented 4 years ago

Really happy to read how this thread went and congrats for Crank! 👏

I just wanted to add a concrete reason (at least for me) to support event listeners "in props" (declarative): consistency.

In the Readme it says Declarative components As you can guess my point is that addEventListener is in the imperative form, so it feels frustrating to see that you don't have a declarative alternative for that, given that event listeners are part of components.

Also, sharing my thoughts about your initial points:

  1. you can support everything you want with a declarative API as well, it doesn't have to be the same as the react API, it just needs to be declarative.
  2. it may happen, but in the train of thought "I will let the user manage render priorities instead of inventing hooks" (with the clear caveat that users may make shitty apps but with the great pro of flexibility for more advanced users), I think it is more consistent too to let the user use event listeners as they want
  3. I don't know if there are issues in modern JS with anonymous callbacks. In the past we had issues specially with stack traces, but this generator based library is clearly not for the past. Am I missing something here?
  4. you don't have to mimic anything, you just created an awesome free form alternative! keep it like that! In fact, I also think now that the React API for this can be improved!
  5. I don't understand this point honestly. Usually you don't pass down event handlers... but that sounds like a different problem really
ghost commented 4 years ago

That's exactly what I wanted to say on this subject... Crank appears to be aiming for declarative components, whereas addEventListener is imperative and that feels like an unintuitive break away from that declarativeness and I guess in writing some tiny toy apps to play around, I haven't really seen where this can be a benefit.

Binding handlers like <button onClick={handleClick} /> makes it very clear, simple, and specific what is happening and something React does right. What element has a click event? That one button. What happens when it's clicked? handleClick is run. Fumbling around introspecting events to filter on tag names, class names, etc, remembering to call preventDefault again, all feels like going in a direction that this library doesn't aim for. If I were using this lib then I foresee myself creating wrappers around my dom elements to get myself back to that kind of declarative react-like API.

gbbarroso commented 4 years ago

Feels like going back to jQuery tbh: if I only look at the HTML (JSX), I have no idea if something there is triggering an event. If I look only at the JS, I gotta cross check with the HTML to see who is the important button that is actually doing stuff. Of course for small components that's never going to be an issue, but we know that someone somewhere is going to be writing big components. Not only that, but having to go all the way to tagName is too verbose (and awkward too).

With React/Vue/Angular, we look at the template and most of the time we can guess exactly what is happening, it saves time, and time is money. Only if needed then we look at the JS implementation (and we know exactly what to look out for).

I like the idea of going back from some design patterns from React, but the event handling was a step in the right direction imo, it's the reason it started existing.

jspears commented 4 years ago

Not trying to beat a horse -- just a bit of spitballing -- If you objection is inline handlers, perhaps use the api to force handlers, to be created outside the function. Then use their placement for selection. So a user doesn't have to decipher the element which was clicked. Having to do your own event selection could be a huge nightmare, especially with deeply nested components, and would encourage jQuery/Backbone style event capturing .

Something like


function *MyComponent ({props}){

  //maybe a symbol?
  const handleClick = this.handler((e:HTMLButtonEvent)=>{
     //scoped
  });
while(true){ 
//and ma\ke you can't invoke functions inside of element attributes..
   yield <button onClick={handleClick}>Click Me</button>
}
}

``

workingjubilee commented 4 years ago

Wild, possibly hare-brained suggestion for an alternative syntax, because it feels vaguely like what aesthetically aligns with Crank's (creator's) desires of adhering closely to DOM semantics while also satisfying the goal of a declarative design:

<SimpleButton Events={ click: function() { /* do stuff */ } }>
<AbsurdlyComplexButton Events={ 
  click: {
    listener: function() { /* do stuff */ },
    options: { /* set options*/ },
    useCapture: true,
    wantsUntrusted: true
  }
}>

Obviously, you would probably not inline all that, but I figured I'd bring out the maximal ugly of my own proposal for everyone to see, rather than guess. I assume you've all seen gnarly React (or Angular, or Vue...) codebases and know, deep in your heart of hearts, that the components there do not necessarily look better.

There is something charming about the simplicity of this, I think, and besides, nothing says It's Just JavaScript like shoving random stuff in a hash map like it's 1995. :wink:

ryhinchey commented 4 years ago

I really love the idea of usingCustomEvent and it's pretty clear you need to keep a door open for it in crank. React's events api (while beautiful) doesn't handle CustomEvent objects. I'm excited about where these two worlds can cross with crank.

That said, I find having to check tagName in order to dispatch events from nested elements a bit cumbersome. Would it be crazy to add a selectors api to this?

function *Clicker() {
  let count = 0;
  const button = this.querySelector('button');
  button.addEventListener("click", (ev) => {
      count++;
      this.refresh();
  });

  while (true) {
    yield (
      <div>
        The button has been clicked {count} {count === 1 ? "time" : "times"}.
        <button>Click me</button>
      </div>
    );
  }
}
DavidWells commented 4 years ago

Thanks for this onclick on element example. It works great in the code sandbox examples 😃

When playing around with this in code sandbox, I added onClick (because JSX/react is burned into my brain) but it wasn't working... After a little trial/error I discovered the lowercase onclick works.

Any thoughts on supporting the camelcase event handlers (e.g. onClick etc.)?

While onClick is purely syntax sugar, I think it would help folks port over existing React components into crank. It might also help when folks run into the "why is onClick not working issue I had 😅

function handleClick(e) {
  alert('clicked')
}

// onclick works
async function ComponentA({throttle = false}) {
  return (<div onclick={handleClick}>Click me</div>)
}

// onClick doesn't work. But it could right? 😃
async function ComponentB({throttle = false}) {
  return (<div onClick={handleClick}>Click me</div>)
}
shirakaba commented 4 years ago

I just came across fancy-emitter today; it does some curious stuff with Promises that might fit into Crank’s idioms (or not), so I thought I might as well link it in case it sparks some ideas for API design options:

https://github.com/mothepro/fancy-emitter

workingjubilee commented 4 years ago

While the difference of onclick vs onClick is seemingly tiny, there is a danger that, if onClick works as-is, people might expect the full React API to be supported. Vue does not support onClick, it uses v-on:click, for instance. Angular has its own approach, as does Ember, et cetera. No other framework (excepting the "mini-Reacts") copies React so exactly on this, even when they do support JSX.

That said, I also think that if literally just desugaring camel case for DOM event assignment was supported, so onClick works (because onclick does), it wouldn't necessarily prompt a full "bank run" on the framework's design. Going to generators is already a big step to ask people, so removing small pains (and onClick is probably 80% of event handlers) would help, and people can see already things are very different. An error would have to be thrown if both onClick and onclick were assigned, though, as that would become valid, and thus have to be explicitly barred. Thus, "make onClick work" is not actually a zero-problem change, and that is ignoring other design choices that Crank might make which people might expect to also be transparent with onClick.

Setting that aside, I think it would be useful to distinguish three points raised in this thread: 1) React's modeling of events is not great and denies you access to the DOM's full power. 2) But it is declarative... and offering declarative vs. imperative syntax is important, regardless of whether it is mostly sweetener. 3) Inlined functions are aesthetically displeasing, yet naming things is hard.

Conceptually, inlined functions in JSX vs. named functions is more of a linting concern: the API could accept or refuse them, and they could be controlled by other means. As a community grows, Brian could express an opinon and offer a common linter setting, which people could reject, while it would still exert a mild normative pressure... the equivalent of going "tsk tsk". Because it would save some people headaches, but cause a few others, that's an appropriate amount of pressure to exert, rather than building it into the framework's core library directly.

Whether the API accepts a declarative syntax is part of the underlying API logic, however: otherwise the API, which is our fancy way of saying "the way we talk to it", is simply different, procedurally managing versus expressing desire. Maybe it's mostly sugar, but who doesn't run their Krebs cycle on that sweet, sweet glucose? Thus why I proposed the events JSX attribute, as it demonstrates you can offer a declarative syntax while still de-sugaring transparently to the addEventListener method, addressing some of the problem that React has by just... getting out of the way.

ryhinchey commented 4 years ago

Having both on* and this.dispatchEvent is really nice. You get something super clean like:

function* Clicker() {
  let count = 0;

  const onClick = ev => {
    count++;
    this.refresh();
  }

  while (true) {
    yield (
      <div>
        The button has been clicked {count} {count === 1 ? "time" : "times"}.
        <button onclick={onClick}>Click me</button>
      </div>
    );
  }
}

And as noted here https://github.com/bikeshaving/crank/issues/15, you can use the custom event system to do redux-ish stuff and in general have more control over events when needed.

scastiel commented 4 years ago

Just experimenting a little (thanks for the great work, Crank looks awesome!), but I think this naive button component implementation can do the trick to have a classic onClick prop if you want to:

function MyButton({ onClick, ...props }) {
  const id = Math.random().toString(); // just need a unique ID

  if (onClick) {
    this.addEventListener("click", event => {
      if (event.target.id === id) {
        onClick(event);
      }
    });
  }

  return <button id={id} {...props} />;
}

Using it just feels like classic React afterward:

function* Counter() {
  let counter = 0;

  const onButtonClicked = event => {
    console.log("event", event);
    counter++;
    this.refresh();
  };

  while (true) {
    yield (
      <p>
        Counter: {counter}
        <MyButton onClick={onButtonClicked}>+</MyButton>
      </p>
    );
  }
}

It seems to work very well… Am I missing something?

Edit: actually I think the result is exactly the same as using onclick instead of onClick as proposed here in @brainkim’s comment… 😇

shirakaba commented 4 years ago

Edit: actually I think the result is exactly the same as using onclick instead of onClick as proposed here in @brainkim’s comment… 😇

Yep, best to stick with the DOM Level 0 event handlers (onclick) rather than use React's camel-cased onClick "synthesised" event handlers (I'm saying "synthesised" because they're not calling an event handler method on the element itself – there's some custom under-the-hood handling being done in the renderer to remap the prop name to the DOM Level 0 event handler name).

By sticking to DOM Level 0 event handlers, users don't have to make their own <MyButton> implementations as you've demonstrated, and neither does CrankJS have to supply custom TypeScript typings for all the DOM elements; nor does it have to remap all the event handlers to camelcase under-the-hood, with custom logic to add & remove the listeners upon props change, just to satisfy a feature request.

It also means that elements can easily be extended to inherit the same event handlers, and that the event name – with correct casing – can be deduced simply by removing the "on" prefix from the event handler property name.

ryhinchey commented 4 years ago

By sticking to DOM Level 0 event handlers, users don't have to make their own <MyButton> implementations as you've demonstrated, and neither does CrankJS have to supply custom TypeScript typings for all the DOM elements; nor does it have to remap all the event handlers to camelcase under-the-hood, with custom logic to add & remove the listeners upon props change, just to satisfy a feature request.

Agree with this! Providing camel cases event names does make it seem more “ react like”; however, that comes as a cost. As with everything in software there’s a trade off. There are plenty of developers who will try Crank without having done React first.

By sticking to DOM level 0, Crank can keep its code base smaller and more focused while providing an event system that’s been a standard for decades.

brainkim commented 4 years ago

I updated the docs to reflect the onevent callback props. Some more thoughts:

  1. I value the feedback, and I think it’s important for me to treat the Crank community with a big tent approach, allowing people to express themselves with Crank in the way they see fit. I’m a very opinionated developer, and I implemented the EventTarget API because I wanted to use custom events, but that’s no reason for me to disallow callback props, and I’d rather convince people with reason rather than by intentionally hobbling APIs.
  2. Callback props are convenient, but one question I have is how do people test them? One advantage of using EventTargets is that you can dispatch mock events on them so you don’t need the full DOM implementation.
  3. One notion I want to challenge is that passing a callback to an element is somehow “declarative” whereas using a method call is “imperative,” or that event delegation is somehow more prone to bugs or less type-safe. Thanks to React, we have an entire generation of developers who are completely illiterate when it comes to basic DOM stuff, and who don’t think it’s necessary to learn them because of the constant slander React devs sling like the idea that the DOM is slow or ill-specified. This simply isn’t the case in 2020, and I’d like Crank devs to have at least a passing familiarity with DOM APIs, because they’re useful and powerful. Like hands up, how many of you knew that DOM elements have a property called tagName and that its value is in all-caps? Crank’s philosophy with regard to DOM stuff is that there’s nothing special about the DOM, and that it is a side-effect just like any other.
  4. I don’t want to provide camel-cased onevent props for the reasons that others have said. Firstly, you have to realize that prop updating logic is the hottest-running code in the codebase, and checking for camel-cased props, while cheap on its own, would have to run for every prop for every element in your application. Secondly, there’s a mental overhead to having to remember the way React camel-cases event props. Off the top of your head, can you tell me how to case onkeypress, ondblclick or oncanplaythrough? Now imagine you’re a non-native English speaker. I like doing less work, and Crank’s approach to DOM element props is to be more transparent and not doing stuff like converting case, or magically adding px to numbers, for instance.
  5. Please, please, please, please, please, please, please, please, please consider using the EventTarget API before porting something like Flux/Redux over to Crank. I’m telling you that CustomEvents can be strongly typed, and my intuition is that the whole reason Redux came into existence is because there was no way to bubble events in React.

@workingjubilee

While the difference of onclick vs onClick is seemingly tiny, there is a danger that, if onClick works as-is, people might expect the full React API to be supported.

Yes exactly! Crank is a big opportunity to address what irked you about React, big and small, and a current competitive advantage is that we can break things without affecting a giant ecosystem. Let me know if there’s anything else from React which you didn’t like and we can work on making it better.

Feel free to continue the conversation! I am closing for housekeeping purposes.

geophree commented 4 years ago

I massaged the TodoMVC example into a more "redux"-ish form, but still using the same underlying functionality. See discussion at #71 and/or implementation at:

https://codesandbox.io/s/crank-todomvc-alternative-jluse https://gist.github.com/geophree/a317c658fd6d7bc51e1e80aa5243be7d

brainkim commented 4 years ago

Reopening for visibility and to promote more discussion I guess 😄. I’ll close it when there are multiple dead on both sides and we begrudgingly decide to sign a treaty.

brainkim commented 4 years ago

One thought I had in a dream last night (yes I am dreaming about responding to GitHub issues and I have been having weird dreams since the quarantine started). One thing I didn’t like about the great compromise is that it means components which wish to support both APIs are required to dispatch events in two ways (once by invoking prop callbacks and once by dispatching event). I had the idea that we could actually modify this.dispatchEvent so that if it found any prop which matched on + event.type, it would call that prop with the event as well. Does that make sense? In short, we’d have a way to support callback props on components without the pains that I outlined in point 4 from this comment, and you’d be able to support both APIs simultaneously. So concretely you could write:

function *MyInput(props) {
  this.addEventListener("input", (ev) => {
    this.dispatchEvent(new CustomEvent("appinput", {
      detail: {
        value: ev.target.value,
        id: props.id,
      },
    }));
  });
  for (props of this) {
    yield <input {...props} />;
  }
}

And in a parent component event props would just work with or without the bubbling of the component like:

<MyInput onappinput={callback} />

This could be a good feature to implement if someone is interested.

monodop commented 4 years ago

It's an interesting idea, but I'm still curious how we can strongly type custom events. You mentioned it can be done, but I haven't yet seen how you'd go about that. It's sort of a big sticking point for me wanting to use it in an application.

shirakaba commented 4 years ago

Just in case it sparks anything, the creator of React Native Web proposed these APIs for comment, one of which concerns event handling, today:

image

https://twitter.com/necolas/status/1255671143857446918?s=20

ryhinchey commented 4 years ago

I had the idea that we could actually modify this.dispatchEvent so that if it found any prop which matched on + event.type, it would call that prop with the event as well.

@brainkim to be honest, I don't like the proposal. It creates a bit of magic that while convenient, might create more harm than good (especially in the early stages of the framework). You've mentioned executional transparency before and I think this removes some of that.

For now, I like sticking as close to what jsx and the dom provide out of the box. I know people dislike writing this.addEventListener and some feel it's a step backward; however, it's immediately clear what I'm listening to in the example below and just because it's called addEventListener doesn't mean it's a step backward :)

I think if your proposal was to land, it would be one more way for people to get confused as to how to write Crank components without providing any net new functionality.

function *MyInput(props) {
    const oninput = (ev) => {
      this.dispatchEvent(new CustomEvent("appinput", {
        detail: {
          value: ev.target.value,
          id: props.id,
        },
      }));
    };

    for (props of this) {
      yield <input oninput={oninput} {...props} />;
    }
  }

function App() {
    this.addEventListener('appinput', (event) => {

    });

    return (
        <MyInput />
    }
}
brainkim commented 4 years ago

@ryhinchey are... are you saying you like this.addEventListener now? 😗

ryhinchey commented 4 years ago

@ryhinchey are... are you saying you like this.addEventListener now? 😗

@brainkim I never disliked addEventListener! I liked having it from the beginning and my PR https://github.com/bikeshaving/crank/pull/57 didn't remove it. In fact, I stated The power of having an event emitter that limits event bubbling to parent components is super powerful 😃

What I still dislike is this https://github.com/bikeshaving/crank/blob/master/src/events.ts#L54-L84. Looping through all children and adding/removing listeners. That said, it's obviously not a show stopper for me and you and others have brought up legitimate examples of using it (some with rxjs).

brainkim commented 4 years ago

Crank 0.2.0 implements the EventTarget and delegation stuff without any dependencies, so I’m even more hesitant to change this API. It’s very little overhead, and it allows component hierarchies to be defined more flexibly. Closing the issue for housekeeping purposes, but feel free to continue discussion.