pH200 / cycle-react

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

cycle-driver is now less convenient or possibly deficient #16

Closed jmale closed 5 years ago

jmale commented 9 years ago

the "get" method used to be callable in a similar way to the cycle.js DOM driver, e.g.

drivers.DOM.get('button.my-button', 'click')

This used to return an observable which you could then filter/map etc. It now returns undefined for anything other than ':root', for which is returns a Subject:

drivers.DOM.get(':root')

To get the same affect as before, you'd need to do something like:

DOM.get(':root').flatMap((div) => { return Rx.DOM.click(div.querySelector('button.my-button')); }).publish().refCount()

Is this something which should be fixed?

pH200 commented 9 years ago

Hi, @jmale. I'm surprised you found that breaking change in the driver, because I thought not many people have used it :)

Because the selector API has been removed #8, it made the Cycle driver pretty much unnecessary right now. The only thing it does now is React.render. And developers are supposed to observe the interactions inside of CycleReact.component. In conclusion, I'm probably not going to put the selector API back to the driver. The selector API cannot subscribe the custom events that emitted from the customized component. If I put it back it would make the dev experience inconsistent.

Also, although I think you made a pretty good example showing how to observe the events, it could still leak, unfortunately. Because we cannot observe sub-tree updates in React, the DOM.get(':root') observable will not trigger onNext for the sub-tree only updates. The better way to do this would be:

DOM.get(':root')
  .flatMap(div => Rx.DOM.click(div))
  .filter(ev => ev.target.matches('button.my-button'))
  .share()
HighOnDrive commented 9 years ago

Really thought the following quote from the Cycle.js site Model-View-Intent page was brilliant. Just diving into things further today and it seems that getting the perfect tradeoffs between Reacts focus on components and Cycles focus on interaction and the whole framework is tough. However, it should not just be tough because of an already rigid React tradition (where interaction was poorly integrated to begin with), we have to stick to the better path :)

START OF QUOTE

Why CSS selectors for querying DOM events?

Some programmers get concerned about DOM.get(selector, eventType) being a bad practice because it resembles spaghetti code in jQuery-based programs. They would rather prefer the virtual DOM elements to specify handler callbacks for events, such as onClick={this.handleClick()}.

The choice for selector-based event querying in Cycle DOM is an informed and rational decision. This strategy enables MVI to be reactive and is inspired by the open-closed principle.

Important for reactivity and MVI. If we would have Views with onClick={this.handleClick()}, it would means Views would not be anymore a simple translation from digital model to user mental model, because we also specify what happens as a consequence of the user’s actions. To keep all parts in a Cycle.js app reactive, we need the View to simply declare how the visual representation of the Model is. Otherwise the View becomes a Proactive component. It is beneficial to keep the View responsible only for declaring how state is visually represented: it has a single responsibility and is friendly to UI designers. It is also conceptually aligned with the original View in MVC: “… a view should never know about user input, such as mouse operations and keystrokes.”

Adding user actions shouldn’t affect the View. If you need to change Intent code to grab new kinds of events from the element, you don’t need to modify code in the VTree element. The View stays untouched, and it should, because translation from state to DOM hasn’t changed.

The MVI strategy in Cycle DOM is to name all elements in your View with appropriate semantic classnames. Then you do not need to worry which of those can have event handlers, if all of them can. The classname is the common artifact which the View (DOM request) and the Intent (DOM response) can use to refer to the same element.
HighOnDrive commented 9 years ago

Here is something more to think about: https://github.com/channikhabra/yarr/tree/4-delegated-events

pH200 commented 8 years ago

The cyclejs driver support has been removed for 3.0, because our driver is not really helpful in my perspective. However, I would glad to put it back if anyone come up with a good idea for the API which integrates between cyclejs and our cycle-react.

pH200 commented 8 years ago

@HighOnDrive With the support of lifecycle events from 3.0, we can now make the interaction collection like Cycle.js' DOM driver:

function makeSelectorInteractions(self, lifecycles) {
  const node$ = lifecycles.componentDidMount
    .map(() => ReactDOM.findDOMNode(self))
    .shareReplay(1);
  return {
    get(selector, eventType) {
      return node$
        .flatMap(node => Rx.Observable.fromEvent(node, eventType))
        .map(ev => ev.target.matches(selector))
        .takeUntil(lifecycles.componentWillUnmount);
    }
  };
}

const MyElement = Cycle.component('MyElement', (_1, _2, self, lifecycles) => {
  const interactions = makeSelectorInteractions(self, lifecycles);
  return interactions.get('.click-btn', 'click')
    .startWith(1)
    .scan(count => count + 1)
    .map(count => (
      <div>
        <button className="click-btn">Click</button>
        <h4>Count: {count}</h4>
      </div>
    ));
});

However, I still prefer that we don't change the interactions API. I've explained this in #8. The reason our API was designed this way is not because it's a better abstract. It's because it has better integration with normal React components.

HighOnDrive commented 8 years ago

Thanks for the delayed update! Not sure you have looked at Cycle.js lately, it just keeps getting more elegant and powerful by the mile, no patchwork necessary to make it go. Here is what André Staltz tweeted on Dec 11th about the next major release, coming just in time for Christmas 2015!

"ANY Cycle.js app can be easily reused as a component in a larger Cycle.js app. Any. That's the highlight of the upcoming version."

Have a Merry and Reactive Christmas :-)

pH200 commented 8 years ago

Thanks for mentioning. I really didn't look at Cycle.js recently. So I just took a look and I found it does get better with the new isolated feature. Not mentioning Andre always commits good code day by day. And the isolated feature makes more sense compared to the custom elements. I'll keep my eyes on and see if we can borrow some good ideas from Cycle.js.

However, I'd like to mention that Cycle-React components have been able to host another Cycle-React component since day 1. Though we had diverse APIs soon after Cycle.js evolved to the driver model. Maybe I'll explain why Cycle-React was not a useful driver for Cycle.js in the future. But right now I would like to talk about another reason we ditched the select API that was used in Cycle DOM driver.

Until the next version of Cycle.js, your DOM.select (or interactions.get) won't respect the scope of the custom element. It was solved by attaching an namespace to the className of the root element, and when the DOM.selects gets the elements it would filter these elements by traversing each of their parent elements and see if they had the correct namespace in the className. It works and it could be done in React by using cloneElement. However, I'd rather not using this hacky approach. It brings too much complexity to the flow. I didn't make this library to change the internal of Cycle.js by switching "virtual-dom" into "react", it wouldn't make any sense.

And there is one more reason. Right now Cycle-React handles the events in the most native and performant way. It wouldn't be if we decided to use the DOM events. Nevertheless there's still rooms for Cycle DOM driver getting more efficient, by fixing cycle-dom#11. Right now every update in the DOM driver (and the hosted driver) triggers a new querySelectorAll. This can be fixed by tapping into the diff/patch process of virtual-dom or even make a whole new virtual-dom/patch for DOM driver. However, Cycle-React could never do such thing unless we can alter the process of React's rendering engine. And I won't make such changes even if I could. Because it will bring too much hacky code to the library while benefits so little to the actual code written with Cycle-React.

corps commented 8 years ago

Just to say, I add my support behind @pH200's reasoning. At my work, I use Cycle-React over Cycle.js for exactly the two main points you make here:

  1. The interactions api makes integrating with other react components simple. The scoping, the expected interface, etc. While React's use of DOM even handling and view abstraction may not be perfect, it's simply a non starter to suggest adding friction to mixing cycle style reactive design with more traditional imperative React. Right now, it's completely transparent to any new engineer who knows the DOM and javascript, and maybe some React, where to get started using or modifying a Cycle-React component. The beauty of the intention <-> view cycle can be achieved in isolation, rather than "all or nothing", and this is a virtue.
  2. Less hacky. I have been following Cycle for awhile, too, and while I agree in principle with its design, its implementation is still questionable. There many edge cases that must be handled with increasingly more involved code, and there are still bugs in core features. Cycle-React is so damn simple and small, and it functions by riding as close to proven React and Rx codebases.
sartaj commented 8 years ago

Hi,

I do like the argument by @pH200 and @corps about the simplicity factor. But I can't seem to wrap my head around 1 thing.

In cycle-react, what should you do in lieu of drivers for non dom technologies, ie. httpdriver, localstoragedriver, urldriver, etc?

pH200 commented 8 years ago

@sartaj Like Cycle.js. You treat them as sinks. I don't have the clear example code right now (will do it later), but it should be like using the Observable from the model as input and interactions from the Root component as output.

You can check these two files: https://github.com/pH200/cycle-react/blob/master/examples/web/todomvc/app.js https://github.com/pH200/cycle-react/blob/master/examples/web/todomvc/local-storage-sink.js

This sink only cares about the input. But that should explain the idea.

sartaj commented 8 years ago

@pH200 cool. In the same TodoMVC app, you have a separate file todo-source.js that basically acts as a localstorage source. So to use it as a 'driver' or 'clean way' so to speak, should these two files be 1 function that both acts as a source and sink?

pH200 commented 5 years ago

For design reasons, namely performance and native React implementation, driver implementation is very unlikely to happen now. Luckily, cycle.js does have its own React (driver)[https://www.npmjs.com/package/@cycle/react])