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

Ability to invoke function inside rivets expressions #554

Closed
stalniy opened this issue Jan 29, 2016 · 18 comments
Closed

Ability to invoke function inside rivets expressions #554

stalniy opened this issue Jan 29, 2016 · 18 comments

Comments

@stalniy
Copy link
Contributor

stalniy commented Jan 29, 2016

It's not very hard to implement ability to pass arguments (and watch them) into function in rivets expression. So that, all the methods will have more clear api

<div rv-show="canShow(todo)" rv-each-todo="todos"></div>

<a rv-click="selectTab(tab)">Tab 1</a>

<h1>{ formatDate(todo.createdAt, 'hh:mm') }</h1>

Basically we need to add one more if inside TypeParser which will compile and return function. We can restrict this so the implementation won't be very complicated. For example, it's possible to pass only primitives and keypaths. That means we can't pass function invokation as a result. So this is not valid:

{ formatDate(todo.getDate(), 'hh:mm') }

However I believe even this case can be handled using recursive.

@blikblum, @Leeds-eBooks, @jccazeaux do you think this something useful or overcomplicated?

@jccazeaux
Copy link
Contributor

@stalniy I can only say 👍

Calling functions in rivets expression is one of the most important missing capability IMHO.

Before coding it we should be sure to see all consequences

  • be sure to handle it right in events binders (not call the function before the event is emitted)
  • as you say, how far we go? One level or recursively
  • databinding must be connected on parameters

@blikblum
Copy link
Contributor

I'm in favor of this

@Duder-onomy
Copy link
Collaborator

I have to say I do not like this. Having been forced to work on Angular apps, this leads to allot of bad code. In massive projects, I have only ever seen this go sour as it allows people who are not careful to make things incredibly difficult to debug.

That being said, It is not a feature that I, or my juniors, need to use. If you can figure out the Coffee Script. I say take a stab at it.

My questions, would formatDate just be an function on rivets.formatters? Would rivets subscribe to the expression todo.getDate(). I believe it would be against how rivets currently works if you are attempting to bind to an expression and not just a keypath.

I would prefer to see, because it matches typical rivets keypath subscription.

<span> { formatDate(todo.date, 'hh:mm') }</span>

@stalniy
Copy link
Contributor Author

stalniy commented Jan 29, 2016

@Duder-onomy good point! It's easy to subscribe to function's arguments but when function doesn't have arguments then we will need to specify its dependencies somehow, so that rivets can track them too.

This sounds a bit complicated. As far as I know currently computed properties (i.e. functions) with dependencies are supported in this way:

<span rv-text="fullName < firstName lastName"></span>
var user = {
  firstName: 'John',
  lastName: 'Does',
  fullName: function() {
    return this.firstName + ' ' + this.lastName;
  }
};

So, despite that this is a bit inconvenient this can be left as it is. So, what we have:

  1. When function expression is written inside event handler `rv-click="selectTab(tab)"
    • no observations needed as callback will receive the latest values when user does an action (e.g. clicks)
  2. When function expression is written inside binder and does not have dependencies
    • it just works as one-time binding rv-text="user.getName()" (one time binding)
  3. When function expression has arguments and inside binder (rv-text="formatDate(user.birthDate)")
    • rivets needs to observe when arguments are changed and update the binding
  4. When function expression has dependencies (rv-text="user.fullName() < firstName lastName)
    • we need to specify them as earlier using < symbol, so that rivets can observe changes
  5. When function expression has dependencies and arguments and inside binder
    • we needs to specify dependencies using < symbol, and rivets should observe both dependencies and arguments

@Duder-onomy
Copy link
Collaborator

Nice,
I like the number 2 and 3. Though, I can guarantee we will get allot of new issues, people will say "I have this binder, but it does not update." Then we will get those coveted green bars closing issues...but screw it. Helping people is the funnest part of open source (IMO)

I agree that expressing a computed property via the old rivets syntax < is a little awkward, and difficult to explain to new people. Using a function call with its dependencies expressed as keypaths, will make computed properties MUCH easier to explain and may even encourage people to use them more.

However, I think that the concept of formatters expressed as function calls can be improved. Take this formatter I see people do allot:

// Will uppercase then pluralize the name.
<span rv-text='user.name | uppercase | pluralize'></span>

I imagine if this was expressed as a function call you could either,
nest them:

<span rv-text='uppercase(pluralize(user.name))'></span>

Or write a parent function

<span rv-text='uppercaseAndPluralize(user.name)'></span>

Would love to hear your thoughts on that use case.

The only other item I can imaging creating confusion is bi directional formatting or computed properties.
Consider this input, it will represent the date as a human readable in the DOM then a UTC in the model.

<input rv-value='user.name | asDate'>
rivets.formatters.asDate = {
  read: function(value) {
    return // formatted for user to read
  },
  publish: function(value) {
    return // converted to UTC for model.
  }
}

A bidirectional formatter makes this clear. It has a read and a publish method.

If you did

<input rv-value='asDate(user.name)'>

How would you handle this?

Anyways, Awesome work.
Greg

@blikblum
Copy link
Contributor

To clarify my comment: i'm in favor to implement passing arguments to event handlers like: rv-on-click="doIt(task)"

The other features i think is out of scope of this library

@stalniy
Copy link
Contributor Author

stalniy commented Jan 29, 2016

@Duder-onomy I like as formatters work, so don't plan to change them.
The syntax for computed properties needs to be improved but this is another story.
Regarding last question about rv-value="asDate(user.name)" I think this should raise an exception as function returns a result and there is no destination where it can be written back. It's counterintuitive to write value back to user.name.

@blikblum I do agree with you that the main purpose of this feature should be event handlers but then devs would want to use this inside binders. For example I saw an issue (#366) which asks about ability to hide item inside rv-each-* based on the model. So, the author of the issue would like to do something like this:

<div rv-each-todo="todos" rv-show="canShow(todo)"></div>

What looks pretty reasonable and handy in my opinion.

@jccazeaux
Copy link
Contributor

@blikblum @stalniy Agree too, most usefull is in event handlers.
@Duder-onomy Like you i don't like Angular expressions. But there is a world between compiled angular expressions and the ability to call functions. We must try to keep rivets simplicity.

I have maybe an idea to handle functions, i must think about it today. I will explain it as soon as it's clear (and if it leads somewhere ;) )

@jccazeaux
Copy link
Contributor

Rivets should not execute functions in bindings

I think rivets binding should not call functions automatically before the formatting process. In this example

{{ model.obj.delete | myFormatter }}

Rivets executes the function delete. This won't let us do anything on the function, only on its result.

What if Rivets didn't auto execute functions

If Rivets didn't call the function automatically we could imagine a specific formatter for that

{{ model.obj.delete | args 'param1' model.param2 | call | myFormatter}}
// Or shorter
{{ model.obj.delete | call 'param1' model.param2 | myFormatter}}

The call and args formatters would look like this

// Returns a new function with arguments
rivets.formatters["args"] = function(fn) {
  var args = Array.prototype.slice.call(arguments, 1);
  return function() {
    return fn.apply(null, args);
  };
};
// Calls the function with arguments
Rivets.formatters["call"] = function(fn) {
  return fn.apply(null, Array.prototype.slice.call(arguments, 1));
};

Why two formatters call and args

Why two formatters ? Because in event binder we must not call the function. We can only add arguments

<div rv-each-name='model.names'>
  <button rv-on-click="model.sayIt | args 'Hello' name">Do it</button>
</div>

Tests

You may test it here : https://jsfiddle.net/jccazeaux/ncp295xL/2/ (for now we can only test it on events binders, rivets must be modified if we want to use these formatters on all binders)

Conclusion

With these two formatters we add the ability to call functions. As these are formatters, we stay in the standard rivets mecanism. Observers and databinding will totally work with that.

Any opinion will be welcome !

@blikblum
Copy link
Contributor

Interesting. The elements rivets provides are very flexible

If implementation is not hard, i would like to see a more JS canonical syntax (only for events):

<div rv-each-name='model.names'>
  <button rv-on-click="model.sayIt('Hello', name)">Do it</button>
</div>

@blikblum
Copy link
Contributor

I updated the fiddle to see if dynamic values are used and also the value of this https://jsfiddle.net/dh8mpe7r/2/

@jccazeaux
Copy link
Contributor

JS canonical syntax should be possible, but we see in @Duder-onomy examples that we will have some problems to deal with.
Actual syntax is pretty easy to learn (only a couple of concepts) and leads to many (if not all) possibilities. Introducing additional JS canonical syntax could lead to leaning (and therefore documentation) difficulties

Is it worth it?

@benadamstyles
Copy link
Collaborator

Sorry I haven't contributed to this discussion yet, but I have been reading it. Do you guys know about the scope property? It makes @blikblum 's example redundant, because in the model.sayIt function you have access to the scope which contains the name property. I think this whole idea of calling functions with arguments might be a red herring, taking us away from the original intention of rivets which is to provide an unopinionated data binding layer. With computed properties and the scope, I can't think of a single situation where you even need to pass or subscribe to arguments.

@benadamstyles
Copy link
Collaborator

Also, as @Duder-onomy hinted at, we shouldn't be encouraging hard-coding primitive arguments in HTML anyway, it is really dangerous practice to have JS logic in a different place to your JS. If you want to call a function with a specific string, make that decision in your JS, for example by checking the scope to decide what the argument to your event handler function should be, or by referencing a partially-applied function in your binder. If you want to do something to a function, rather than just to its result, you should do that in JS, not in HTML. This isn't React 😛

We can already invoke functions in rivets, we can already watch computed properties, we can already check in the function what part of the model we're in by querying the scope... I know it's not always immediately obvious how to do these things but I don't think adding more syntax is the best solution. Just better docs probably! Sorry if I seem like I'm always being negative but I use rivets in all my projects and I think it is fantastic, there's always a way to do what you need and I'm really wary of ruining it by trying to make the HTML more like JS. I think there's a reason Mike chose pipes rather than parentheses...

@stalniy
Copy link
Contributor Author

stalniy commented Jan 31, 2016

@Leeds-eBooks having an ability to invoke functions:

  • in event handlers leads to clearer API and easier testing. Imagine that selectTab option even if you change the order of args that way that scope is passed first in tests you will need to run it this way selectTab({ tab: tab }). Moreover in this example view leaks into model/controller so now selectTab depends on existing of tab variable inside view
  • in expressions leads to more clear understanding of is going on in the view. That is really unclear why rivets calls passed property as function { user.name } seems like a property binding but name is function and it's invoked

Frankly speaking, I believe this syntax exists as a bit a hacky way of achieving the goal of function invocation.

@jccazeaux
Copy link
Contributor

@Leeds-eBooks I mainly follow your opinion. We must not add JS-like syntax. But we should authorize function execution with arguments in any binder, not only in events binders.

It would be very simple, we just have to remove auto execution of functions. As this auto execution can cause strange behaviour (see issue #557) we should consider it.

@jccazeaux
Copy link
Contributor

Is this issue still necessary since #582 ?

@benadamstyles
Copy link
Collaborator

I think it isn't, so I'll close. However it makes me think that perhaps there should be a test for the call formatter in rv-on-* binders, do you want to do that, or I can?

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

No branches or pull requests

5 participants