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

Rivets function auto-execution in expressions #571

Closed
jccazeaux opened this issue Feb 5, 2016 · 19 comments
Closed

Rivets function auto-execution in expressions #571

jccazeaux opened this issue Feb 5, 2016 · 19 comments

Comments

@jccazeaux
Copy link
Contributor

In this issue, I would like to discuss the auto-execution of functions in Rivets expressions

{ model.myFunction }

If myFunction is a function, Rivets will execute it and send the result in the template.

I think we should turn off this feature. It would allow us to manually call the function with parameters. This would be a powerfull feature for a very small cost. The manual execution would be handled by a formatter like

{ model.myFunction | call model.param1 'param2' }

For backward compatibility reasons we could have a parameter in Rivets configuration

rivets.configure({autoExecuteFunctions: true});

I know it's less user friendly than a JS expression. But it's in the rivets template syntax and would totally benefit of the existing databinding mecanism without any additional work.
I like rivets expression syntax, because it's very easy to learn but very powerful.

@jccazeaux
Copy link
Contributor Author

This solution could replace what's called Computed properties (http://rivetsjs.com/docs/guide/#computed-properties). Computed properties are a way functions when a property changes in model. If find this concept hard to explain to developpers (not very natural) and it kinda break the layers isolation because the view must know the internals of the function to add the dependent properties.

With this issue we could replace

<span rv-text="event.duration < start end"></span>

by

<span rv-text="event.duration | call event.start event.end"></span>

There is not much coding difference but now the view doesn't know the internals of duration function. It only knows duration function take two parameters and sen them.

@Leeds-eBooks @Duder-onomy @blikblum If you have time to take look at this issue, your opinion would be welcome

@benadamstyles
Copy link
Collaborator

I have used a custom formatter of my own called call in the past. Perhaps we could define a special syntax for this that doesn't use a formatter, to avoid conflicts? Or, it really is just a simple formatter and rivets simply doesn't auto-execute functions by default. It would be up to the developer to define their own call: (func, ...args) => func(...args) formatter. Or, we include this as a built-in formatter?

EDIT: just want to add that initially I was resistant to this but the more I think about it the more sense it makes. Rivets to me is all about having as little "magic" as possible. Leaving it to the dev to execute their functions via a normal formatter fits this very well.

@jccazeaux
Copy link
Contributor Author

A specific syntax would be great, but a "simple" formatter would be so easy to implement, with no risk of regression

@benadamstyles
Copy link
Collaborator

What do you think about introducing built-in formatters, including call, just like there are built-in binders? There are several that almost every project uses, like equals, gt and lt, not etc.

@jccazeaux
Copy link
Contributor Author

I agree, in our projects we have included these formatters (exactly the ones you mentionned and some more like append, prepend...) as default for developpers.

@benadamstyles
Copy link
Collaborator

Ok so just to clarify/summarise (sorry for repeating): what I think might be the right approach is:

  • Rivets does not automatically call functions (possible config option to control this)
  • A call formatter is included by default that basically does (func, ...args) => func(...args)
  • We also include several other very common formatters by default, just like Rivets already does with standard binders

Does that fit your thinking?

@Duder-onomy
Copy link
Collaborator

FYI, I took a scan through a few of our projects and asked the other teams during standup if they were using functions in the sense described here. Everyone said that they were not doing it, also, most people thought that this was a bad idea anyway and were surprised it was possible.

So, to chime in on behalf of me and all my coworkers, 👍

As far as including more formatters, I think a few would be good but def not too many. The ones you mentioned also appear in all of our projects.

This will probably be a task for me as I am updating the docs right meow for the 0.9 release.
We should teach people how to write formatters to fit their domain. This will ensure we are not dealing with a ton of bugs around 'the gravatar url formatter does not work in my crazy use case' .

This will essentially render the 'computed properties' feature useless.
So lets get some more support as that will be backwards incompatible.
For the 0.9 I would like to see the computed properties feature stay, for now, we throw a console warning saying the new syntax is preferred and will be deprecated in the 1.0 release.

@jccazeaux
Copy link
Contributor Author

Aren't 0.x versions well suited to introduce a new syntax?
To reassure those who want to update, we could handle backward compatibility with a rivets parameter. Something like

rivets.configure({
  'autoExecuteFunctions': true/false // I'm not good in parameter naming, so if you have a better idea...
})

Not the prettiest way, but it would give an option to those who want 0.9 (or 1.0) but don't want to add | call in their templates.

And for formatters, actually there is already something here
We could just reference this page in the docs.

@blikblum
Copy link
Contributor

blikblum commented Feb 8, 2016

FYI semver allows to add breaking changes in 0.x versions

@benadamstyles
Copy link
Collaborator

@Duder-onomy i'm really surprised that none of your colleagues use functions that get called by rivets, it's so useful! But – it is sometimes necessary to refer back to the model while you're reading your jade/html/whatever to work out if a property is a function or a value, and having call there will therefore help with readability.

However, I'm a little concerned about the computed properties issue. I use this feature a lot across my projects, here's a quick example:

<button rv-class-highlight="ready < user.password user.confirm">Submit</button>

It should be self-explanatory but essentially it applies the highlight class to the button when the ready() function returns true, and this ready() function checks the password and confirmation for validity and is called every time they are changed.

The only way I can think of doing this without computed properties is:

<button rv-class-highlight="user.password | checkPasswordAndConfirmation user.confirm">Submit</button>

This actually seems less clear to me, and requires a formatter that is overly specific, rather than keeping the checking logic within the view-model where it probably should be.

Couldn't we just say that if a binding has <, Rivets will auto-execute it? So to call a function, you either have computed properties, or you add the | call formatter?

Although semver says we can break everything whenever we like, I think it's debatable whether Rivets should actually be at 0.x right now anyway. We know for a fact that a) this is being used in many production projects and b) it has been "stable" for well over a year. I think we should proceed as if Rivets is already at 1.x.

@jccazeaux
Copy link
Contributor Author

@Leeds-eBooks i missed you message

  • Rivets does not automatically call functions (possible config option to control this)
  • A call formatter is included by default that basically does (func, ...args) => func(...args)
  • We also include several other very common formatters by default, just like Rivets already does with standard binders

That fits my thinking 👍
For point 3 as @Duder-onomy said we must define too many. But as they are on this page, maybe we can just let projects choose what they want to include.

@jccazeaux
Copy link
Contributor Author

@Leeds-eBooks

Couldn't we just say that if a binding has <, Rivets will auto-execute it? So to call a function, you either have computed properties, or you add the | call formatter?

it's a possibility, as < will have no use if function is not executed. But computed properties cause issues as described in #213. Computed properties can be kept for backward compatibility but should be deprecated in favor of | call

@benadamstyles
Copy link
Collaborator

@jccazeaux Ok I can live with that. Do you agree that in the example I showed above, computed properties are a good solution? Or is there a better way?

@blikblum
Copy link
Contributor

blikblum commented Feb 8, 2016

I suggest to create some snippets with the use cases. And implement them using both Approaches. With computed and with call.

@benadamstyles
Copy link
Collaborator

Well my example wouldn't use call because you're not binding to a function (ready()), instead you have to bind to a string (user.password) so that it updates every time the string is updated by the user. That's why, in this case, computed properties and an auto-executed function make more sense to me, and I think they should maybe remain. My example above without computed properties actually wouldn't work because it wouldn't update if user.confirm changed. I'll paste it again:

<button rv-class-highlight="user.password | checkPasswordAndConfirmation user.confirm">Submit</button>

How would you make this be evaluated when either user.password or user.confirm changed? I don't think there's a way without computed properties.

I think rivets needs a way to evaluate x when y changes, and the only way to do that currently is computed properties. Unless I'm missing something obvious? I think keeping the computed property feature, and only auto-executing functions when the computed property feature is used, is a good compromise.

Alternatively, and I haven't thought about this in depth because I've only just thought of it, perhaps we could keep the computed property feature but not auto-execute, so it would be something like:

<button rv-class-highlight="ready < user.password user.confirm | call">Submit</button>

@jccazeaux
Copy link
Contributor Author

@Leeds-eBooks For you example i would have done

<button rv-class-highlight="ready | call user.password user.confirm">Submit</button>

This means the function ready would take two parameters. I like this approach because

  • With computed properties the view needs to know ready internals
  • With | call the view calls ready with the arguments it needs to do the work
    This is my point of view. I cannot say with | call formatter i would never need computed properties so you're right, we must keep the feature.

For your idea about writing

<button rv-class-highlight="ready < user.password user.confirm | call">Submit</button>

It will work.

@blikblum @Leeds-eBooks
Can we start a branch for this feature? I will send the PR on it and we will be able to test it, create some snippets and finally make our opinion of the results. What do you think?

@benadamstyles
Copy link
Collaborator

Ah, my apologies. I did not know that

<button rv-class-highlight="ready | call user.password user.confirm">Submit</button>

would re-call ready every time user.password and user.confirm changed. This is behaviour that I was not fully aware of.

In which case, my use-case for computed properties is completely fulfilled by the call formatter and actually computed properties don't need to remain. Sorry I got this wrong!

If you submit a PR I think we can merge it, personally.

@jccazeaux
Copy link
Contributor Author

OK I'll submit the PR ASAP

@stalniy
Copy link
Contributor

stalniy commented Feb 13, 2016

Probably this may be closed

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