jsguy / mithril.bindings

Two way automatic bindings for mithril
MIT License
34 stars 3 forks source link

Some comments #1

Open explorigin opened 9 years ago

explorigin commented 9 years ago

There isn't really a great place to give some feedback so I've just made an issue here. You've essentially recreated KnockoutJS with these changes to Mithril.

This is pretty good work but I think a lot of it is against the idiomatic manner intended for Mithril (particularly the Pubsub part which Leo has expressly avoided). While I'm not sure if Pubsub is awesome or terrible Leo left it out because it tends to produce spaghetti and I've seen this in large projects so I respect his intention to leave it out. (Although he has been thinking about changing his mind in this regard.)

You've gone to a lot of effort to implement bindings but I think this could be done much simpler. For example, you have:

m.e('div', {type: 'button', toggle:c.showTheThing}, "Toggle the thing");

But instead of including all your changes, I could define:

m.toggle = function (prop) {
  return function(evt) { prop(!prop()); };
}

and thus do this:

m('div', {type: 'button', onclick:m.toggle(c.showTheThing), "Toggle the thing");

This way you can avoid all the plumbing of the bindings system with the same effect and the code is just as readable. Also in my version you get the flexibility of choosing the triggering event.

Zolmeister commented 9 years ago

How do you deal with multiple event bindings? e.g.

{onclick: m.toggle(), onchange: m.toggle(), onkeyup: m.toggle()}
// vs
{toggle: fn()}
jsguy commented 9 years ago

@explorigin The toggle thing is just something I find useful - you can easily use external functions like you've shown though that is probably a bad example of using bindings - a better example might be if you wanted to have bi-directional bindings. In mithril you would do this:

m("input", { name: "name", onchange: m.withAttr("value", c.model.name), value: c.model.name()}),

However, with mithril.bindings, you would do:

m.e("input", { name: "name", value: c.model.name }),

Which is not only much more succinct, but when working with forms, you actually want to have a bi-directional binding, (in most cases), so that if you change values either in the model or in the input field it will update everywhere else.

Bi-directional bindings are also quite useful in places where you might otherwise write a component, for example:

https://github.com/lhorie/mithril.js/issues/81#issuecomment-53047083

You don't need the select component if using bi-directional bindings - they are much more expressive.

@Zolmeister you could handle change events in the model - you can use "subscribe", for that, however toggle is really only built for the specific use case of clicking on something to toggle a value - I cannot think of a real use case where you'd want keyup to toggle something - you'd probably want to write a custom method, or model property .

Zolmeister commented 9 years ago

@jsguy I think we can do better than overriding default property functionality. If there is two-way binding, it should probably be explicit and optional. e.g. m('input', {bind: model})

jsguy commented 9 years ago

Yeah I know what you mean, my initial idea was to add something like "valBind", however because there is no access to the internal binding method of mithril, I had to create a method to override the element method, so you must use m.e(... to get bi-directional bindings, instead of m(..., so I think that is probably ok. The alternative was to monkeypatch the m function, which I wanted to avoid.

Zolmeister commented 9 years ago

Oh, I see. Seems quite easy to miss the .e at the end

jsguy commented 9 years ago

Yeah it might be - I've added support for bi-directional bindings in sugar tags, so that it will do it automatically. Ideally sugar tags and bindings would be used together.

jsguy commented 9 years ago

I did a presentation on Mithril, (well, a comparison of various virtual-dom based frameworks, internally to where I work), and some of the feedback was:

  1. "It is too nested" - in referece to the templates being completely inline of the app.view by default.
  2. "It is too verbose" - in reference to using m("div", {... compared to something like DIV({...
  3. "It is too complex" - in reference to how bi-directional bindings work when working with largeish dynamic forms

So in order to help with that, I created mithril.templates, mithril.sugartags and mithril.bindings. This not only shows how extensible Mithril is but provides optional "comfort" libraries for those that find the default way of doing things in Mithril off putting and laborious.

I can imagine it might seem slightly over the top/annoying to those who are completely used to writing things in Mithril, but getting more people involved can only be a good thing - most of them know angular/knockout/ember/etc., and if this helps ease them into using the next generation of frameworks, I think it is worthwhile.

explorigin commented 9 years ago

@jsguy, it's helpful to know the background that motivated these projects. However, if Leo allowed everyone's requests, then Mithril wouldn't be small and fast (and idiomatic). All 3 of these complaints should be prefaced with "I don't like it because..." (not you but the people giving feedback). Compaint 3 is perhaps the only one that has real merit.

I don't know what your level of experience is in the webdev world but I'll offer this tidbit: If you try to accomodate everyone's petty preferences, you'll become bitter and burnt out. True leadership comes from picking the right way and staying the course. If you've picked the right way, people will follow. This is wisdom to follow in the volunteer project world. The dynamics are more complex when you're paycheck depends on it.

@Zolmeister multiple event bindings is a moot point because even the mithril.bindings system has to pick one event rather than bind to them all. With it you would have to create a new binding for each different event. Yuck.

The best argument for bindings is simplifing two-way bindings, but all of this flexibility could be wrapped in a function of the options.

m.twoWay = function(opts) {
   // Here we scan opts for binding names and replace them with "value" and "onchange" as appropriate.  Thus avoiding the need for m.e().
}

m('input[type=text]', m.twoWay({value: c.identity), "SSN #");

These are matters of my preference (and experience), I just think that this way is more readable and simpler. But it's your project. Go nuts! (But I would oppose adding these changes to Mithril.)

jsguy commented 9 years ago

I've got no problems with Leo's decision to not include 209 - It would have been much more flexible and allowed a higher level of integration from external libraries - however it would have also come with it's share of headaches from inexperienced developers, and on that ground I appreciate the decision to keep it in app-space rather than core. There was no real way for me to understand what would and wouldn't be able to be pushed into the core until he commented on the code, as there isn't much documentation explaining where the lines are drawn - I think it is always good to start conversations about open source and design - especially limitations on scope and goals of a project.

One thing that might be useful on the front page underneath the "What is Mithril" sections is a "What Mithril isn't", and list how it differs from other comparable libraries, especially about the bindings - all the other frameworks have a more expressive set of bindings - there is a very vague section on safety compared to other libraries but I don't really know why that is on the front page, it would be much more useful to explain how Mithril differs from the rest of the libraries, or something along the lines of what makes it stand out.

About the nesting and verboseness - they are quite valid points - maybe not from a technical standpoint but more from a "will a new user want to use this compared to other libraries", so yeah it is opinionated but most probably shared by the majority of web developers. You could argue that unless it is technical, you should ignore it, but I never feel this is the right attitude.

I like your idea for m.twoWay, it is a good start on how to implementing a better set of bindings - and as mentioned, mithril.bindings will stay in app-space, there is no plans to integrate it in mithril core.

I appeciate your comments about picking the right way - this is very important, however one thing I've found even more important is to be able to re-evaluate what the right way is - after all: thoughts, ideas and opinions you won't be able to have on your own will only come from others.