pH200 / cycle-react

Rx functional interface to Facebook's React
MIT License
370 stars 18 forks source link

Broken VDOM abstraction with intents? #8

Closed peterjoel closed 9 years ago

peterjoel commented 9 years ago

Using a querySelector to access DOM events seems like a broken abstraction. It:

  1. forces you to know the structure of the DOM that a particular component will produce
  2. ties you to DOM events instead of higher-level events and callbacks from the react elements.

I would like to interact with view events like this:


function isRefresh( intent ){
    return intent.name === 'refresh'
}
function mkRefresh(){
    return {name:'refresh'}
}

function component( intentions ){
     let refreshes$ = intentions.where( isRefresh )
     return refreshes$
                    .startWith( null )
                    .scan( n => n + 1, 0 )
                    .map( n =>
                             <div>
                                 <div>Refreshed {n} times</div>
                                 <Button click={intentions.push(mkRefresh())}>Refresh</Button>
                             </div>)
}

This looks a lot like some cycle-js examples that I've seen, where the events are pushed directly onto subjects that are in the view function's scope. The difference is that this still allows business logic to be composed in as many layers as you like, using normal function composition, and those layers can still access the (abstracted) intents.

pH200 commented 9 years ago

The original Cycle.js uses querySelector for handling events, for example: https://github.com/staltz/cycle/blob/master/examples/many/many-intent.js

While I didn't see cycle-js examples using this approach(push events to subjects), I agree with your opinion, actually. In fact, Cycle-React provides an API which does exactly what you suggest: https://github.com/pH200/cycle-react/blob/master/docs/interactions.md#interactionsgeteventsubject

So your example becomes:

function component( interactions ){
  return interactions.subject( 'refresh' )
    .startWith( null )
    .scan( n => n + 1, 0 )
    .map( n =>
       <div>
           <div>Refreshed {n} times</div>
           <Button click={interactions.subject('refresh').onEvent}>Refresh</Button>
       </div>);
}

Again, I support the idea that interactions.get is not a good abstraction for us. Most importantly, it doesn't work well with other native React components. I'll seriously consider deprecating this API. Let me know if you have any thoughts on this change :)

Finally, I think the same issue has been discussed for Cycle.js cycle/#128. And Staltz explained the reason of using selector for events here. Perhaps you can share your thoughts to that issue thread.

peterjoel commented 9 years ago

Done.

pH200 commented 9 years ago

So, that's how it worked for Cycle.js cycle/#128. As for Cycle-React, we want to use the event system from React so the only choice is to take the approach of using inline event handlers. As a result, the interactions API of the next version will be looking like this:

function app(interactions) {
  return interactions.get('onButtonClick')
    .startWith(0)
    .scan(-1, (x,y) => x+1)
    .map(number =>
      <div>
        <p>Counter: {number}</p>
        <button onClick={interactions.listener('onButtonClick')} />
      </div>
    );
}

Like what's being discussed at cycle/#128, the biggest drawback of having the named event collection is that you could have elements emitting named events, but has no subscriber responsible for these named events. Sure, we can throw warnings, but we can't prevent this from happening. However, the same thing applies to Flux architecture by dispatching actions and not observing them. So, I think although we have this downside, but it's an approach that has been proven to work.

pH200 commented 9 years ago

The new interactions API has been implemented for 1.0

Migration guide can be found at changelog

peterjoel commented 9 years ago

Looks good!