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

Big API change for Views and Intents #73

Closed
staltz opened this issue Jan 21, 2015 · 13 comments
Closed

Big API change for Views and Intents #73

staltz opened this issue Jan 21, 2015 · 13 comments

Comments

@staltz
Copy link
Member

staltz commented Jan 21, 2015

Just an idea for now.

This

var HelloView = Cycle.createView(model =>
  ({
    vtree$: model.get('name$').map(name =>
      h('div', [
        h('label', 'Name:'),
        h('input.myinput', {attributes: {type: 'text'}}),
        h('h1', 'Hello ' + name)
      ])
    )
  })
);

var HelloIntent = Cycle.createIntent(view =>
  ({changeName$: view.events('.myinput', 'click').map(ev => ev.target.value)})
);

Notice no event concerns in View, and Intent using view.events('.myinput', 'click'). Implementing this in the framework will be a challenge, since (1) it is suicidal to make event streams for all possible events and all possible VirtualNodes in the vtree, (2) the intent doesn't have direct access to the Renderer, where we insert the actual callbacks that feed DOM events into the view's event streams.

One early idea is to use event delegation using the vtree's top-level VirtualNode as parent to the querySelector happening in view.events('.myinput', 'click'). Let VP be the vtree's top-level VirtualNode, and P the rendered correspondent in the actual DOM. The View magic could give a uid to VP, the Renderer magic would set an attribute using uid as value on P, and Inject magic would do event delegation to catch events on the child matching '.myinput' under P, and forward them into an observable living in the view, which the intent dynamically generated when calling view.events(). This implementation relies on the vtree of a View having always only one top-level VirtualNode. Intent magic would have to touch the DOM for doing event delegation, but that's Ok since anyway interaction events only make sense in the context of a DOM, and Intent anyway get raw DOM events.

Things to keep in mind:

  • Event delegation for custom events coming from custom elements
  • Where app developers would put event.preventDefault()?

Another possible implementation: make the Renderer be a representative of the User. Maybe even rename Renderer to User, but that could be confusing. The idea is to make the Renderer a side-effectful DataFlowNode instead of a DataFlowSink. It would return event streams of raw interaction. Would be a simpler and less hacky way compared to the first approach above. Resulting usage example:

var HelloModel = Cycle.createModel(intent =>
  ({name$: intent.get('changeName$').startWith('')})
);

var HelloView = Cycle.createView(model =>
  ({
    vtree$: model.get('name$')
      .map(name =>
        h('div', [
          h('label', 'Name:'),
          h('input.myinput', {attributes: {type: 'text'}}),
          h('h1', 'Hello ' + name)
        ])
      )
  })
);

var HelloIntent = Cycle.createIntent(user =>
  ({changeName$: user.events('.myinput', 'click').map(ev => ev.target.value)})
);

var UserInteraction = Cycle.createRenderer('.js-container');

HelloIntent
.inject(UserInteraction)
.inject(HelloView)
.inject(HelloModel)
.inject(HelloIntent);

Things to consider:

  • How would we solve this in custom elements, where the renderer is implicit (created inside registerCustomElement())?

References: http://davidwalsh.name/event-delegate

@gaearon
Copy link

gaearon commented Jan 21, 2015

I'm not using Cycle so my say probably doesn't count, but delegation by classes is painful.
I'm so happy I don't have to do it anymore in React..

@staltz
Copy link
Member Author

staltz commented Jan 21, 2015

@gaearon Care to share a bit why and how it was painful in React?
(note also that class names would not be used globally here, but limited to the scope of one View's vtree)

@gaearon
Copy link

gaearon commented Jan 21, 2015

Rephrasing: I used to use event delegation a lot before React and it was painful.
Now that I'm using React, I don't have to do it anymore, and pain is gone. :-)

When I was using Backbone, I would often:

  • Forget to update event selector name when refactoring CSS because I used the same classname in CSS.
  • Remove selector (and element), but forget to remove event handler because it's not immediately in sight when deleting element.
  • Add another element with same CSS selector when I didn't mean for it to have the same JS event handler.
  • Once elements with two different classnames need to have the same handler, I need to add yet another selector to both of them (and remember to update selector-handler mapping from two old classnames to one new classname).
  • Finally, it's always one more lookup when locating handler from element tree. I have to look at the element first, then at delegation table, and then find the handler.

Most of these are solved by using a consistent prefix like .js- and never, ever using those in CSS, but it's too tempting to do it the easy way that most people will repeat the same mistakes I outlined above.

Note: not all of these problems are applicable to Cycle at all. In fact, probably, there isn't much difference because in Cycle's approach, you can't have inlinish handlers anyway because Cycle is reactive.

But at least, with $ convention, it's visible what is part of view's public contract. With the approach you're suggesting in this issue, no CSS refactoring (or even DOM restructuring: some people may use tag names in selectors) can be done safely without looking at both view and all intents observing it, because you can't remember if this particular classname was also used for event handling.

I'd be against leaking CSS class names or even DOM structure to intents in any way.

@staltz
Copy link
Member Author

staltz commented Jan 23, 2015

Thanks @gaearon for the valuable input, it's appreciated.

Most of the problems you see arise from the facts that (1) CSS selectors can be used globally, (2) CSS files are used for styling.

These problems are not relevant to Cycle, since (1) view.events('.myinput', 'click') would search for '.myinput' only under view, and not globally, (2) my vision for styling in Cycle is to not use any CSS file, and to do it all with inline styles. This way, Cycle is aligned with React's future vision of doing all the styling in JavaScript, inlined: https://github.com/reactjs/react-future/blob/master/04%20-%20Layout/04%20-%20Inline%20Styles.md . That way, selectors will not be used for styling, and we are free to use selectors for other cases, such as for this issue.

@gaearon
Copy link

gaearon commented Jan 23, 2015

Yup, I understand about class search being confined to a single view. I don't see yet how this solves the issues I outlined but perhaps I miss something..

I agree (2) makes some of these problems go away though.

@cem2ran
Copy link

cem2ran commented Feb 3, 2015

I really like this approach! Separate those concerns 👍 Wouldn't adding a key to each element similar to what react does and virtual-hyperscript allows effectively eliminate the need and problem of using classNames?

@staltz
Copy link
Member Author

staltz commented Feb 3, 2015

Not sure yet if we can use keys, I suspect we need to use class names because the Intent is the one selecting interaction events, not the Renderer nor the View, and the Intent would only have access to the DOM. And we cannot grab the vtree between View and Renderer since that would end up changing the API in leaky abstraction way.

@cem2ran
Copy link

cem2ran commented Feb 3, 2015

I should have been more precise :)

So what we want to accomplish is to establish a mapping between a specific user interaction on an element and the resulting intent. The problem is that we have to be able to define which interactions on which element are relevant for a given intent.

I believe people are quite confident using the querySelector API so why not just use this to it's full extend and also generate keys for our DOM tree ala react so that we have an index of our element within our (V)DOM. This would also allow us to easily define a click action on list items as such:

var MovieSelectionView = Cycle.createView(model =>
  ({
    vtree$: model.get('listofLists$')
      .map(listOfLists =>
        h('div', /*listOfLists.map( list => h('ul' .map('li' ...*/ [ //omitted the use of map to show the result:
          h('ul#watchList', {attributes: {data-id: "0"}}, [
            h('li', {attributes: {data-id: "0.0"}}),
            h('li', {attributes: {data-id: "0.1"}}),
            h('li', {attributes: {data-id: "0.2"}}),
          ]),
          h('ul#recommendations', {attributes: {data-id: "1"}}, [ 
            h('li', {attributes: {data-id: "1.0"}}),
            h('li', {attributes: {data-id: "1.1"}}),
            h('li', {attributes: {data-id: "1.2"}}),
          ])
        ])
      )
  })
);

var MovieSelectionIntent = Cycle.createIntent(user =>
  ({
      watchListSelection$: user.events('#watchList', 'click').map(ev => ev.target.getAttribute('data-id')),
      recommendationSelection$: user.events('#recommendations', 'click').map(ev => ev.target.getAttribute('data-id'))
  })

//OR

  ({
      movieSelection$: user.events('ul > li', 'click').map(ev => ev.target.getAttribute('data-id'))
  })
);

The query selection would of course be scoped to the vtree's top-level VirtualNode as you mentioned.
Am I missing something or?

The above example is perhaps not that pretty. Thinking out loud at this point. But one could perhaps prefix data-id with the model name? ex. "listOfLists$.watchList.0", where watchList would be object within our listOfLists object instead of an array named listOfLists.

@qur2
Copy link

qur2 commented Feb 7, 2015

Reading from far away and I thought that:

  • @gaearon 1st point is still relevant because you use the class name in the intent and in the view when rendering with hyperscript. So you still need to remember to update in two locations at least.
  • I like what @cem2ran wrote.

@staltz
Copy link
Member Author

staltz commented Feb 7, 2015

@qur2

So you still need to remember to update in two locations at least.

Currently in Cycle v0.11, you also need to remember to update in two locations, since a user event stream (such as mouseDown$) is mentioned in both View and Intent. This proposed change wouldn't remove that, it would remove just the View's responsibility in determining user events.

@qur2
Copy link

qur2 commented Feb 8, 2015

OK, thanks for the clarification.

On Sat, Feb 7, 2015, 17:05 André Staltz notifications@github.com wrote:

@qur2 https://github.com/qur2

So you still need to remember to update in two locations at least.

Currently in Cycle v0.11, you also need to remember to update in two
locations, since a user event stream (such as mouseDown$) is mentioned in
both View and Intent. This proposed change wouldn't remove that, it would
remove just the View's responsibility in determining user events.


Reply to this email directly or view it on GitHub
#73 (comment).

@staltz staltz self-assigned this Feb 10, 2015
@staltz
Copy link
Member Author

staltz commented Feb 10, 2015

Note to self:

Proposal for Model-View-"User"-Intent in custom elements:

             // function, not createView. And notice element here
var HelloComponent = function(element, Properties) {
  var HelloModel = Cycle.createModel((Properties, Intent) =>
    ({
      name$: Properties.get('name$')
        .merge(Intent.get('changeName$'))
        .startWith('')
    })
  );

  var HelloView = Cycle.createView(Model =>
    ({
      vtree$: Model.get('name$')
        .map(name =>
          h('div', [
            h('label', 'Name:'),
            h('input.myinput', {attributes: {type: 'text'}}),
            h('h1', 'Hello ' + name)
          ])
        )
    })
  );

  // Notice createRenderer replaced with createDomUser
  // essentially the same thing, just renamed, except it should also
  // have the method .events(selector, eventName)
  var HelloUser = Cycle.createDomUser(element);

  // Notice the Intent is injected a User
  var HelloIntent = Cycle.createIntent(User =>
    ({changeName$: User.events('.myinput', 'click').map(ev => ev.target.value)})
  );

  HelloIntent
  .inject(HelloUser)
  .inject(HelloView)
  .inject(HelloModel)
  .inject(Properties, HelloIntent);

  return {
    // notice no more vtree$ exported here
    // this part is only used for custom events exported out of this component
  };
};

staltz pushed a commit that referenced this issue Feb 11, 2015
staltz pushed a commit that referenced this issue Feb 11, 2015
staltz pushed a commit that referenced this issue Feb 11, 2015
staltz pushed a commit that referenced this issue Feb 11, 2015
@staltz
Copy link
Member Author

staltz commented Feb 11, 2015

Done in v0.12

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants