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

Event expressions - RFC #2386

Merged
merged 12 commits into from May 27, 2016
Merged

Event expressions - RFC #2386

merged 12 commits into from May 27, 2016

Conversation

evs-chris
Copy link
Contributor

@evs-chris evs-chris commented Feb 16, 2016

This is my attempt to resolve #1172, #1396, #2311, and partially #2176. It is also somewhat tied to #2369.

What this does

This takes event handling and unifies it as an expression instead of having special parsing for handling method events. This opens event handling up to everything possible with an expression in Ractive. Here're a few examples:

  • call data methods e.g. on-click=".loadStuff()"
  • call more complex structures e.g. on-click="@this.reset.partial('foo')" or on-click=".foo().and.bar(., 'yep')"
  • call parent methods for tightly integrated (container) components e.g. on-click="@this.parent.doAThing(.)"
  • chain multiple actions in an event handler e.g. on-click="@this.toggle('foo') && @this.fire('fooed')" or since these are parsed as a list on-click="@this.toggle('foo'), @this.fire('fooed')"
  • the code is a lot smaller and generally easier to follow as a result

I have also taken the spread args and made it available anywhere, which is what ES2015 allows e.g. on-foo="fn('hi', ...arguments, 42)" is perfectly legal. It is also extended to any other call expression with any reference e.g. {{ doThing(foo, ...bars, baz, ...bats) }}. I implemented this by half-shimming with an IIFE based on Babel's output. There may be a better way, but this didn't require an extensive refactor.

Additionally, dollar args ($1) and arguments refs are now allowed to have children as per #2311. These are handled in the same way as event refs with children.

Comments specifically requested

Updated

  1. Method events are handled in their current state my detecting basically ^someName(.*)\s*$, prepending @this., and handling as an expression. This seems to work pretty well in practice, but is it confusing that toggle('foo'), .toggle('foo'), and ~/toggle('foo') mean potentially very different things? With the confusion surrounding where method events actually call, should they just be deprecated and eventually removed in favor of @this.method(args)? Deprecating and removing plain method call events would eliminate the remaining inconsistency in template reference resolution. Consensus is that non-specific calls should be removed, so they are deprecated here and would be removed later.
  2. I haven't addressed proxy events in this PR yet, because I'd like some feedback first. I would prefer to support only no-args events (option 3) for auto-proxying, which would be super simple to support here. Is that what most people would like to do? The only other thing I consider tenable from a parsing (code and mental) viewpoint is to support on-click="nameExpression:argsList". That would make on-click="{{ foo ? 'bar' : 'baz'}}: {{bop}},bip,{{ { oh: 'my' } }}" become on-click="( foo ? 'bar' : 'baz' ): bop, 'bip', { oh: 'my' }". Argumented proxy events are deprecated here and would be removed later.
  3. I also haven't addressed decorator and transition args yet, because I'd like some feedback first. I would prefer to drop mustaches in the directives entirely and just move to comma separated expression lists for decorators and transitions. That combined with as-decoratorName and transitionName-in/transitionName-out/transitionName-in-out (or something similar) and the changes in Second swing at conditional directives: Revenge of the Fragment - RFC #2369 would allow that. This would be more consistent with event directives. Another proposal has been to treat them as method calls instead e.g. decorator="decoratorName(arg1, arg2)" which seems reasonable, but I think it's less consistent and could get weird with transitions with their in/out/in-out facets. This is addressed in Second swing at conditional directives: Revenge of the Fragment - RFC #2369.
  4. How should deprecation be handled? I'd kinda like for this to land for 0.8 now, since it's still compatible with 0.7 and would let the big cleanup start immediately after 0.8.

Status

  • Handle method events as expressions
  • Account for spread, arguments, and dollar args in expressions
  • Allow children of dollar args and arguments
  • Allow spread args in any invocation (it's there, so may as well use it, no?)
  • Tests , tests, and more tests - the deprecation path leaves the old proxy handling code in place, so only expression-related stuff is tested beyond the existing event tests
  • Finalize deprecation path
  • Clean up code mostly deferred until after deprecation cycle - there's lots of code that can go away and/or be nicely refactored

@fskreuz
Copy link
Contributor

fskreuz commented Feb 17, 2016

With the confusion surrounding where method events actually call, should they just be deprecated and eventually removed in favor of @ractive.method(args)?

I'm in favor of this one. But I'd suggest @instance. @ractive looks like a reference to the Ractive global. Thinking something like @ractive.version and stuff like that.

I haven't addressed proxy events in this PR yet, because I'd like some feedback first. I would prefer to support only no-args events (option 3) for auto-proxying, which would be super simple to support here. Is that what most people would like to do?

Method calls all the way! Burn the old event syntax! 🎆 🔥 🚒

Another proposal has been to treat them as method calls instead e.g. decorator="decoratorName(arg1, arg2)" which seems reasonable, but I think it's less consistent and could get weird with transitions with their in/out/in-out facets.

I method calls, possibly just args I guess?

<div transition="transitionName({ in: { duration: 5 }, out: { duration: 10 }})></div>

...or maybe multiple setup calls

<div transition="transitionName('in', 5); transitionName('out', 10);"></div>

Not sure how transitions are written, just throwing in ideas.

How should deprecation be handled?

Partial support in 0.8 with warning log, remove from code by 0.9. Isn't that the usual mo?

Even though it's longer, @ractive.toggle('foo') is much more clear to me when looking at a template.

Explicit is always better than implicit. 😄

@guilhermeaiolfi
Copy link
Contributor

What @fskreuz said. @ractive doesn't seem to imply the current instance. @this was discarded? @current maybe?

@fskreuz
Copy link
Contributor

fskreuz commented Feb 17, 2016

@guilhermeaiolfi this means the current context in templates (synonymous with .). Using @this to mean the current instance can make it confusing.

@heavyk
Copy link
Contributor

heavyk commented Feb 18, 2016

I really @instance to me, it seems much more intuitive than @ractive -- which I would assume would be the same as Ractive - as in Ractive.extend()

@evs-chris
Copy link
Contributor Author

I originally went with @ractive because just about every piece of code I've seen in the wild using Ractive has var ractive = new Ractive(...) somewhere towards the top. I hadn't considered that @ractive would point to the constructor because it's all lower case. I would think that if that were the case, it would be @Ractive based on convention (jshint, eslint, etc). Also, I think of @instance as a bit generic, so it doesn't stand out as strongly as a Ractive instance to me.

At any rate, if that is to be changed it needs to be soon, because @ractive has already been out in edge for about two months but would only have been used by people who watch PRs pretty closely. This is probably as good a place as any since it is proposed to make the special ref mandatory for event methods... anyone have any strong preference? After some consideration, I still prefer @ractive.

@sabob
Copy link
Contributor

sabob commented Feb 18, 2016

For me <div on-click="foo()"> is nicer than <div on-click="@ractive.foo()">, but only because it has been entrenched that foo() sits on the Ractive instance.

I understand the need to align events to resolve similar to references, so I think this is a good change overall.

Once folk gets used to calling @ractive.foo() maybe we can revisit shortening it to just @foo(). I appreciate the arguments against adding symbols to the template, but once @ractive.xxx is entrenched, maybe we would appreciate a shortcut more ;-)

@heavyk
Copy link
Contributor

heavyk commented Feb 18, 2016

actually, I kind of like @sabob 's idea that all @ would reference the instance which is similar to coffeescript's convention to use @foo -> this.foo. but since the @ is reserved for special references (like @global), maybe instead what if it were @@ so, @@foo() would be the same as (what is now) @ractive.foo()

EDIT: just to be clear, I am using edge and I do have @ractive sprinnkled about my components - and I'm also ok with no change

EDIT2: another idea is @self

@fskreuz
Copy link
Contributor

fskreuz commented Feb 18, 2016

@evs-chris I'm convinced 😄 Noted down in the docs repo to document the different @ things.

@sabob @heavyk The use of @ isn't native to JS so it might be weird to newcomers. Not everyone writes CoffeeScript either. I think it should be left as @something instead of standalone @ to give it more meaning.

@heavyk
Copy link
Contributor

heavyk commented Feb 18, 2016

yeah I agree with @something convention now that I think about it.. I guess my my order of preference would probably be:

  1. ~/foo()
  2. @self.foo() or @instance.foo()
  3. @ractive.foo()

@martypdx
Copy link
Contributor

  1. Using @something to refer to component instance

    a. I agree with @evs-chris that var ractive = new Ractive(...) provides a cognitive connection to @ractive.

    b. Current method events, while now familiar to us, don't fit with the rest of data refs in ractive. Looking at: on-click="clicked( foo )" why would I know that foo referred to data context but clicked referred to component context? When @ractive means the current component instance across the board then it should be easier for new people coming to ractive (though a bit of a migration road bump for us old-timers :) ): on-click="@ractive.clicked(foo)".

    c. We will get tired of typing @ractive.foo and @ractive.root.user and it will become an eyesore in our templates to have @ractive all over the place, which is why I introduced Ractive @state RFC #2345 to introduce @foo as a shortcut for @ractive.foo and @@foo as a shortcut for @ractive.root.foo. Yes, it makes the built-in @keypath, @index a bit odd - but reserved keywords are nothing new.

  2. Proxy-event shortcut: Agree that having on-click="handler" as a shortcut for on-click="@ractive.fire( 'handler', event )" (assuming I'm understanding @evs-chris correctly) makes a lot of sense. Events bubble and fit a set of use cases different than method handlers. With the use of this.event, a very large percentage of events can be dispatched this way. Having it be a shortcut for @ractive.handler() doesn't really save you much, especially if you can write @handler()

  3. Plugin syntax
    a. Agree that if all element plugins use the attribute name to communicate the plugin type and name it makes things more consistent: on-click, as-tooltip, fade-in.
    b. Event handlers are fundamentally different than other plugin attribute values because they are arbitrary expression to respond to an event, whereas decorators and transitions are intialization values for the plugin. It will make decorators and transitions more terse if the attribute value is interpreted as an argument list: as-thing='expression' where '[' + expression ']' is what gets "interpreted" in the parser and passed to fn.apply( ractive, args ) when actually evaluated. It enables both single or double value passing via ="foo" or =" qux, 'BAR' " as well asoptionspattern js:="{ foo, bar: 'BAR' }".

  4. Begin the deprecation warnings!

@heavyk
Copy link
Contributor

heavyk commented Feb 19, 2016

@martypdx hmmm, I actually never cognitively connected that inconsistency with the data and instance methods (and I've written a lot of ractive now!!). I wonder how I missed that.

so what you're proposing then is for it to instead be @clicked( foo )?

TBH, I could dig that... I do like that syntax introduced in #2345, a lot 👍

@evs-chris
Copy link
Contributor Author

@martypdx regarding 2, after the deprecation, yes, I would have the parser rewrite the argument of on-click="([^(]+)" as @ractive.fire($1, ...arguments). In other words as long as it's obviously not a call, it would reproxy the named event with any arguments passed (including event, if present).

@guilhermeaiolfi
Copy link
Contributor

@fskreuz considering the syntax proposed in: #2345. I think @this would make a lot of sense instead of @ractive. The same way that this.foo would ref the foo in the context and @foo would ref the foo in the ractive instance. this would ref the context and @this would ref the instance. Consistency!

@martypdx
Copy link
Contributor

@guilhermeaiolfi I'm starting to like to the idea of @this too

@heavyk
Copy link
Contributor

heavyk commented Feb 19, 2016

let's just alias @this to @ractive and call it good. everyone wins and no deprecations for us old-timers :)

@JonDum
Copy link

JonDum commented Feb 19, 2016

I've got to third the @this.

@JonDum
Copy link

JonDum commented Feb 19, 2016

Although, If we do go with @this, do we even need the @?

call data methods: on-click="loadStuff()" or on-click="../loadStuff()" or on-click="~/loadStuff()"
call proto methods: on-click="this.loadStuff()"
call complex structures on-click="this.reset.partial('foo')" or on-click="this.foo().and.bar({{this}}, 'yep')"
call parent methods on-click="this.parent.doAThing({{this}})"
chain multiple actions on-click="this.toggle('foo') && this.fire('fooed')"

It kinda works, and avoids the @ completely, but uses {{ }} mixed in to pass expressions... which may be inconsistent with how expressions are used in other places (but this inconsistency already exists < 0.8).

@martypdx
Copy link
Contributor

Although, If we do go with @this, do we even need the @?

Mixing mustaches inside an expressions has been a PITA, both from an implementation perspective and IMO as a developer because you end up in "mixed-mode" javascript and html template land. The goal is to use mustaches for html and text interpolation, allow javascript expressions inside mustaches and inside plugin attribute directive values.

The other benefit to use a marker instead of assuming the first token of an event handler applies a method name is that the expression itself can more more dynamic as multiple actions can be invoked:

on-click="@this.set( 'x', foo ), @this.set( 'y', bar )"
on-click="@this.push( 'items', newItem ).then( _ => @this.set( 'newItem' ) )"

[note: AFAIK, neither of those examples is currently supported even with this PR, @evs-chris?]

While arguably there is an initial learning in having both this and @this, @ would be recognizable as a special keyword. Ultimate syntax test would be something like:

{{#each items}}
<li on-click="@this.select( this )">{{this.name}}</li>
{{/each}}

I was going for succinctness in shortening that to @select but I could live with @this.select and it avoids the javascript weirdness of concatting obj.prop down to objprop.

@evs-chris
Copy link
Contributor Author

@martypdx no, but on-click="@ractive.set('x', foo) && @ractive.set('y', bar)" is. I don't know what @richharris' stance on arrow functions in the template is, but plain functions where explicitly excluded. I've found myself wanting simple, single statement arrows for mapping and the like before, but it's usually better to pull those out of the template. In-template functions are an entirely different 50 gallon drum of worms, though.

@martypdx
Copy link
Contributor

no, but on-click="@ractive.set('x', foo) && @ractive.set('y', bar)" is

That's what I thought, but that's a bit of a hack and feels error prone when people use them with functions that have a falsey return. Maybe allow the comma operator so this expression can be properly, um, expressed: on-click="@ractive.set('x', foo), @ractive.set('y', bar)"

In-template functions are an entirely different 50 gallon drum of worms, though.

yep. and future drum of worms for sure.

@evs-chris
Copy link
Contributor Author

It's not exactly the comma operator per se, but I added support for a list of expressions. The last value in the list is the one used for cancelling.

@evs-chris
Copy link
Contributor Author

Coming back to it after a bit of time has elapsed, @this still looks good, so I'm going to leave it at that.

I've also updated the intro comment with the result of the other bits of discussion here. The general consensus seemed to be that this is a good way forward, so any thoughts on getting this in for 0.8 or 0.9 (or not at all)?

@guilhermeaiolfi
Copy link
Contributor

So much goodness to wait until 0.9. Since it's ready, ship it. 0.8 from me.

@JonDum
Copy link

JonDum commented Apr 28, 2016

0.8, ho!

@fskreuz
Copy link
Contributor

fskreuz commented Apr 28, 2016

0.8 is good from this end 👍

@martypdx
Copy link
Contributor

martypdx commented Apr 29, 2016

I've been coming back to something @guilhermeaiolfi said:

  1. this (without @) to always ref the ractive instance and let just .foo to ref the context. It's more aligned with OOP. @this/@ractive/@me/whatever seems a little off considering others symbols like @Index and @key.

If we put aside the BREAKING CHANGE aspect, I prefer an API where {{this}} always referred to the ractive instance, both in mustache references and in event handlers.

You could still use {{.}} to mean the model context. In fact you could argue it's more consistent {{@this}} means current context, like current @Index, @key, @keypath. Plus with aliases in 0.8 you have other options for naming model contexts beyond "this".

Thoughts?

@evs-chris sorry about being 11th hour :)

@JonDum
Copy link

JonDum commented Apr 29, 2016

@martypdx No complaints about that. I concede to whatever Chris and you like there.

If you follow that train of thought, shouldn't all data/context refs start with @ and everything else be reserved keywords?

So...

@global -> global
@root -> root
@this -> this
this -> .'

To remain consistent

@evs-chris
Copy link
Contributor Author

@martypdx well I'm not really for it, but I'm not really against it either. I like that @ stuff stands out as 'not in my data' in the template. I typically use the . form of scoped references e.g. .foo rather than this.foo, so it doesn't really affect me on that front. I don't know how often people use this in the wild, but it's not in either mustache or handlebars as far as I can tell. From what I've seen from issues, more people tend not to use scoping at all. It looks like this was added in b74fb4d so it's been around for a while with its current meaning.

All that said, I think it would be pretty easy change to make. Anyone else (especially @Rich-Harris)?

@martypdx
Copy link
Contributor

martypdx commented Apr 29, 2016

@evs-chris, @JonDum for the record, I'm not actually advocating @this get added under that scheme, just pointing out the option exists if people think {{.}} is inadequate.

@martypdx
Copy link
Contributor

martypdx commented Apr 29, 2016

btw - self is the apparently the preferred way to get to global scope across browser, web and service workers, and node. I wonder if we might keep things simple via these mapped changes:

this -> .
@global -> self
@this -> this
@root -> this.root

Only first item would be breaking change as others have not been introduced.

@heavyk
Copy link
Contributor

heavyk commented Apr 30, 2016

good point (especially the service workers), I think that mapping to is quite reasonable.

is there some way that could be combined in some way with your state idea in #2345 too? maybe we can define our own @ vars? or, heaven forbid a @state variable? (prolly not) I've had multiple occasions now, where having access to state would have been a more desirable approach (even though it's simple enough to add state from an external object into the template)

@fskreuz
Copy link
Contributor

fskreuz commented Apr 30, 2016

Thread becoming long, adding just one more :trollface: to hopefully seal the deal. Let's all agree to this proposal:

  • Anything without @ means it's something related to data or a valid-ish JS expression.
  • Anything with @ means it's something Ractive-specific, helper keywords etc.

It becomes clear when you put it like this:

I like that @ stuff stands out as 'not in my data' in the template.

#2386 (comment)

this is a meta reference that has something to do with where I am in a template construct and nothing to do with data

#2386 (comment)

That should be enough of a guideline to avoid confusion. We should steer away from keyword-specific to a broader view (had to re-read the thread and my silly comments to realize). A doc page for @-things should also be sufficient, sort of like a glossary.

Really looking forward to 0.8 (as if I weren't using edge all this time).

@martypdx
Copy link
Contributor

@evs-chris @fskreuz @this it is!

I think we should merge this now, because it changes or supports a number of other features that are already in edge (like @ractive)

@martypdx martypdx merged commit eb6c7d5 into dev May 27, 2016
@kouts
Copy link

kouts commented Jun 10, 2016

Is the '@this' prefix required from now on?
Does writing on-click="fire('eventNameToBeFiredOnThis', event)" still work?

@guilhermeaiolfi
Copy link
Contributor

@kouts yes, it works without @this: http://jsfiddle.net/p9hbej0k/

@martypdx
Copy link
Contributor

@kouts, @guilhermeaiolfi Though it is deprecated if you check the console:

Ractive.js: Unqualified method events are deprecated. Prefix methods with '@this.' to call 
methods on the current Ractive instance.

whereas default firing by name w/o args is still supported:

on-click="eventNameToBeFiredOnThis"

@kouts
Copy link

kouts commented Jun 10, 2016

So in order to be future proof should I add the '@this' to all my event calls inside my templates?
Wouldn't it be better if the '@this' part was optional?

@guilhermeaiolfi
Copy link
Contributor

Yeah, I think I said that somewhere. Not just for events, for properties in general it would be nice to avoid using '@this'. I would like to avoid those features that differ ractive from pure javascript. The exception being when there is analog way of doing that in javascript.

@evs-chris evs-chris deleted the event-alt branch June 13, 2016 20:20
@JonDum
Copy link

JonDum commented Jun 23, 2016

I've been going through all my components preparing them for this change.

Now having used it extensively... I'm really not a fan of all the clutter. 90% of the time I'm just doing a set(). It's far more uncommon for me to have methods in my data.

Wouldn't it be better if the '@this' part was optional?

+1 to that. If you did want to call a method on your data, can't you use .foo(), ../foo(), ~/foo() anyways? If we don't deprecate the old syntax and just let it remain everything works pretty dandy:

http://jsfiddle.net/ddxgxupr/

(except for the {{#with foo}} {{ .test() }} {{/with}} that doesn't appear to work, may just be a bug)

@evs-chris
Copy link
Contributor Author

I'm personally opposed to anything that makes the same reference resolve to different things in different contexts. My designer buddies have a hard enough time with one set of rules 😆

I think we should land something like #2345 that, among other things, makes @set('foo', bar) equivalent to @this.set('foo', bar). Would that get close enough?

@kouts
Copy link

kouts commented Jun 24, 2016

I've already switched to @this, but an alternative is always nice to have.

@dagnelies
Copy link
Contributor

dagnelies commented Jun 24, 2016

well, I'm not fan of @this because I still think people will confuse it with this, which have totally different meanings.

Now you can do this:

<button on-click="@this.do(this)">foo</button>
<button on-click="this.do(@this)">bar</button>

If I'd be a noob, I'd just be confused 😲 ...especially since there are already @index and @key which are common in templates.

My 2 cents.

@heavyk
Copy link
Contributor

heavyk commented Jun 28, 2016

I'm a huge fan of the #2345 PR (but more for the state part about it). the @ operator feels natural to me cause I've spent a lot of time writing livescript. it's also the same in coffee-script: @set 'lala', 2 is shortcut for this.set('lala', 2)

that being said, I believe @JonDum brings up a good point. in my code, I almost never use @ variables in the template (cept when I need to access global state) and I've always used ~/foo() syntax instead. so, perhaps the @ variable isn't really needed so much. I dunno... personally, I think ~/foo() is the most 'natural' for me, cause ~/ makes me think of referring to $HOME in bash.

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

Successfully merging this pull request may close these issues.

Discussion: should we deprecate the old proxy event syntax?