ractivejs / ractive

Next-generation DOM manipulation
http://ractive.js.org
MIT License
5.94k stars 397 forks source link

Dynamic proxy event problem on edge #2849

Closed Jeff17Robbins closed 7 years ago

Jeff17Robbins commented 7 years ago

Description:

Dynamic proxy event names (see doc) bug using conditional sections on Ractive-edge.

Error thrown:

2017-01-30 08:54:58.222 ractive.js:5030 Uncaught ParseErrorcharacter: 15line: 1message: "Expected matching quote '"' at line 1 character 15:↵<h1 on-click="{{#if a.b}}func1{{else}}func2{{/if}}">hello world</h1>↵              ^----"name: "ParseError"shortMessage: "Expected matching quote '"'"stack: "Error: Expected matching quote '"' at line 1 character 15:↵<h1 on-click="{{#if a.b}}func1{{else}}func2{{/if}}">hello world</h1>↵              ^----↵    at ParseError (

Versions affected:

0.9.0-build-65

Platforms affected:

Chrome 55.0.2883 / Windows 10

Reproduction:

works on 0.8.10 https://jsfiddle.net/rsL7qhss/

broken 0.9.0-build-65

https://jsfiddle.net/pLsdv1gp/

Code:

the conditional section resulting in a dynamic proxy event name no longer works on 0.9.0-build-65

var r = new Ractive({
    el: '#target',
    template: '<h1 on-click="{{#if a.b}}func1{{else}}func2{{/if}}">hello world</h1>',
    data: {a: {b: 1}}
});

maybe worth noting: this kind of conditional section does work on 0.9.0-build-65:

var r = new Ractive({
    el: '#target',
    template: '<h1 on-click="{{#a.b}}func1{{else}}func2{{/}}">hello world</h1>',
    data: {a: {b: 1}}
});
evs-chris commented 7 years ago

That's one of the deprecations that were removed in 0.9. Trying to parse what may be a string or mustaches followed by what may be strings comma separated, a string with commas in it, or strings and mustaches caused all sorts of werid issues, so the much more regular <h1 on-click="@.fire(a.b ? 'func1' : 'func2', event)">hello world</h1> is the current preferred way. That will likely stay the preferred way going forward, though I've been trying to find a clean way to reintroduce some sugar for proxy events. The only planned change is to deprecate/remove event and replace it with @context and @event, where @context is a handle to the current context and @event is the actual event object. See #2810.

I've probably missed a few places in the docs for edge, but I'm planning another docs sweep later this week.

evs-chris commented 7 years ago

Thinking about a proxy event replacement construct, how about something like <a proxy-click="event name expression, ...event args"> where the event is automatically injected into a call to fire. Other names that look promising are fire-click, when-click, and on-click-fire, so fire-${eventNames}, when-${eventNames}, and on-${eventNames}-fire. That would make the example here one of:

<h1 proxy-click="a.b ? 'func1' : 'func2'">hello world</h1>
<h1 fire-click="a.b ? 'func1' : 'func2'">hello world</h1>
<h1 when-click="a.b ? 'func1' : 'func2'">hello world</h1>
<h1 on-click-fire="a.b ? 'func1' : 'func2'">hello world</h1>
<!-- vs <= 0.8 -->
<h1 on-click="{{#if a.b}}func1{{else}}func2{{/if}}">hello world</h1>
<h1 on-click="{{a.b ? 'func1' : 'func2'">hello world</h1>
<!-- vs edge/0.9 -->
<h1 on-click="@.fire(a.b ? 'func1' : 'func2', event)">hello world</h1>

Also, for the non-if form that doesn't error on edge, it will actually fire an event named "{{#a.b}}func1{{else}}func2{{/}}", which is probably not at all what you're after.

Jeff17Robbins commented 7 years ago

This specific deprecation caught me by surprise, as I've been trying to keep up with Ractive and didn't know that we had to worry about losing conditional mustache dynamic proxy events. Not to belabor it, or try to make my surprise your problem ;-), but where was this announced and/or discussed? I don't see it directly discussed in #2810.

Here's the asymmetry that has me still puzzled:

var r = new Ractive({
    el: '#target',
    template: '<h1 style="{{#if a.b}}color:green{{else}}color:blue{{/if}}">hello world</h1>',
    data: {a: {b: 1}}
});

https://jsfiddle.net/wsvrLvmn/

works.

And I'm really hoping that you're not about to tell me that this conditional attribute value is also deprecated!

Since that looks so very very similar to:

var r = new Ractive({
    el: '#target',
    template: '<h1 on-click="{{#if a.b}}func1{{else}}func2{{/if}}">hello world</h1>',
    data: {a: {b: 1}}
});

I continue to experience a sense of disconnect. Yes, I know that style is a real Attribute of the h1 element, whereas on-click is a pseudo-Attribute, configuring something "on the side", so to speak.

But it is so nice and easy to not have to keep that in mind when "coding" my template. It seems a shame to add to my cognitive burden to have to remember that on-click is "special", and so I need to start embedding JavaScript expressions in my template, that seem to have the same semantic role as {{#if}} in the Mustache domain specific language. Especially if I am allowed to Mustache attribute values.

Why make the template author drop out of mustache language into JavaScript? I like Ractive because it lets me keep my JavaScript in one place, and my Mustache in another. Intermingling them starts to feel a lot like React, which already does a good job of an intermixed paradigm.

None of this is meant to imply that I have any detailed appreciation for the implementation issues! I hear you when you say:

Trying to parse what may be a string or mustaches followed by what may be strings comma separated, a string with commas in it, or strings and mustaches caused all sorts of werid issues

But we've built a large application using Ractive, and we assumed that a feature documented in a nice big bold heading style

Dynamic proxy event names

would be safe to use.

BTW, I notice in the deprecation notice this

If you just need an event to be dynamically subscribed, you can place it in an {{#if}} conditional.

I don't understand what that means. What does "just need an event to be dynamically subscribed" mean, and how does that differ from a "dynamic proxy event"? And where does that {{#if}} you mention go?

Sorry for so many questions, but this change has me surprised, and means a bunch of rewrites for us that we didn't expect, since the Mustache support in Ractive seems so central to the point of it.

Jeff17Robbins commented 7 years ago

I just saw your second post:

Thinking about a proxy event replacement construct, how about something like where the event is automatically injected into a call to fire. Other names that look promising are fire-click, when-click, and on-click-fire, so fire-${eventNames}, when-${eventNames}, and on-${eventNames}-fire.

Sorry, but I remain lost. What is wrong with the on-click pattern we are using? Why does it need a replacement? What does on-click now mean?

evs-chris commented 7 years ago

I'm not always great at explanations, so if this doesn't make sense, let me know and I'll try to come up with something better...

For this message, "mustaches" largely means "mustache interpolators" e.g. {{foo}} and not block mustaches ({{#if}}, {{#each}}, etc) that are more structural and generally aren't associated with a single specific value being output. Block mustaches absolutely do not work well with producing values, but they excel at strings and vdom.

Only event directives, decorators, and transitions were affected by the deprecation, or in other words, only Ractive constructs that specifically expect not to be passed plain strings. The issue that brought that change about was that the directive argument syntax was entirely ambiguous, especially around events that may be proxy or method calls, so what you would think would parse might not be what actually parsed. All of those directives as of edge now use the same parsing logic that doesn't include mustaches because the directive value is already in "expression" mode.

You can think of it like <a on-click="{{name}}:{{arg1}},{{arg2}}"> was actually getting a string with whatever values happen to be in name, arg1, and arg2 and then trying to figure out what the actual values were supposed to be based on that string. There are many scenarios where the string gives you something largely unparseable, so you end up with all of the args jammed together in a single string arg. That issue was further compounded by method-call syntax. It boils down to anything with mustaches in it comes out as a string in the end, with a few special exceptions for simpler cases, like the single mustache used for binding. Heaven forbid you try to use a string literal with the same quote as the attribute value.

<a on-click="...expression..."> has the expression part turned into a function that is called with any references in the expression. Since it's a plain JS expression, there's no ambiguity of string vs maybe-a-value. All of those directive args as of edge are simply handled as JS expressions. The only exemption to that is what is left of proxy events that only specify an event name and are designed for use re-proxying component events.

TL;DR for that bit - mustaches turn out strings, expressions turn out anything, so for places where you generally expect anything, expressions tend to work out better. Square pegs and round holes and whatnot. Turning strings into expressions is headache-y and performance killing.

Now regular attributes generally expect either a string or boolean value, and the boolean flavor typically don't have a value e.g. checked in <input type="checkbox" checked />. That leaves the string flavor which work out nicely with mustaches that typically produce strings. Any attribute that is not a directive will still play nicely with mustaches.

As of 0.8, in addition to plain attributes, any directive can be placed in a conditional section, so if you need a dynamically bound event (will be bound and unbound conditionally) you can throw them in a conditional section e.g. <button {{#if someCondition}}on-click="doSomething()"{{/if}}>. That also works with decorators, transitions, and special sometimes-directives like value and checked.

For the deprecation discussion, that mostly happened in the original issue #1172 and PR #2386. It's very difficult for any extensive proxy event support to live in the same directive as method/expression events, but fired events are definitely useful and a bit more painful since expressions landed. That's why I asked about/would like to reintroduce a proxy-specific directive.

JonDum commented 7 years ago

That's a solid explantation I think Chris.

fired events are definitely useful and a bit more painful since expressions landed

😏 I think that's actually only because of forcing @this.

<div on-click="@this.fire('mahEvent')"><\div>: just barely crosses the threshold of gross verbosity.

Versus <div on-click="fire('mahEvent')"><\div>: clean and Ractive-y.

Jeff17Robbins commented 7 years ago

I agree @JonDum , @evs-chris -- thank you for a great explanation! I'm going to try to summarize it the way I heard it, to see if I'm following along correctly.

The special event directive, that syntactically looks like an attrName=attrValue assignment on an element, differs from a regular attribute in a templated element in the following ways:

1) its attrName begins with on- 2) the right-hand side of the =, the attrValue, is an expression, not a mustache, because there are expression-enabled things needed on the right-hand side

So my attempt to do this:

var r = new Ractive({
    el: '#target',
    template: '<h1 on-click="{{#if a.b}}func1{{else}}func2{{/if}}">hello world</h1>',
    data: {a: {b: 1}}
});

https://jsfiddle.net/pLsdv1gp/

fails in edge, because "{{#if a.b}}func1{{else}}func2{{/if}}" is not an expression.

But

var r = new Ractive({
    el: '#target',
    template: '<h1 {{#if a.b}}on-click="func1"{{else}}on-click="func2"{{/if}}>hello world</h1>',
    data: {a: {b: 1}}
});

https://jsfiddle.net/vc95co48/

works in edge, because the entire attrName=attrValue is treatable as a string, and thus fair game to generate via a mustache?

If that is a correct account of the distinction, I think I understand it. Is is as special bit of knowledge a template author has to keep in mind, but I can see how the specialness of the event directive takes precedence over trying to allow use of mustache on the right-hand side of an event directive.

That said, would it be possible to provide a global function in Ractive that would let me explicitly pay the performance price of taking a trip down Mustache lane first -- kind of like an eval(...) function for Mustache?

So that, for example,

on-click="{{#if a.b}}func1{{else}}func2{{/if}}"

could be "saved" by changing it to

on-click="$('{{#if a.b}}func1{{else}}func2{{/if}}')"

(or if the $ is too sickening...)

on-click="meval('{{#if a.b}}func1{{else}}func2{{/if}}')"

with the idea that the meval(...) function (another hideous name, 0 for 2!) indirects the string result of the mustache into the actual expression desired?

Or is that more trouble than it is worth, and we're stuck with a more major rewriting of a bunch of our templates that use dynamic event proxy names? Obviously I'm trying to avoid doing more work than we have to in order to eventually upgrade our app.

evs-chris commented 7 years ago

works in edge, because the entire attrName=attrValue is treatable as a string, and thus fair game to generate via a mustache?

Close 😄. Having an {{#if}} wrapping attributes inside the tag body doesn't really deal with strings. It just controls when the attributes are rendered. When !!a.b it's effectively <h1 on-click="func1"> and when !a.b it's effectively <h1 on-click="func2">. Attributes and directives as of 0.8 are no longer stringy, so they get their own vdom nodes that handle their own rendering. That's as opposed to being a fragment (string) or just a map hanging off an element in < 0.8.

That said, would it be possible to provide a global function in Ractive that would let me explicitly pay the performance price of taking a trip down Mustache lane first -- kind of like an eval(...) function for Mustache?

You could theoretically add one, but in order to evaluate it, you would have to create a new ractive instance, link its root to the current context somehow, get its toHTML, and then fire the resulting event name on the instance. I think that would be considerably more trouble than it's worth if it's possible. If all of your dynamic events take that form, I think a moderately complex regex replace would take care of it in a much cleaner fashion.

src.replace(/on-([^=]+)=["']\{\{#if ([^\}]+)\}\}([^\{]+)\{\{else\}\}([^\{]+)\{\{\/if\}\}["']/g, 'on-$1="@.fire($2 ? '$3' : '$4', event)"')

or something like that.

Jeff17Robbins commented 7 years ago

Thanks for the feedback. I see that my understanding of the VDOM scheme in Ractive is, umm, deficient. Sorry to inflict my ignorance on you!

I appreciate your patient explanations very much. And that regex replace is totally off-the-charts above and beyond! Thank you so much @evs-chris! Name dropping @paulie4 since he understands regex, unlike me.

Jeff17Robbins commented 7 years ago

I'm continuing to try to understand the 0.9 approach to on-click.

I created this example https://jsfiddle.net/m3tywcak/

The template uses on-click two ways: 1) simple proxy event 2) @.fire(...)

<script id="template" type="text/ractive">
<button on-click='func1'>Click 1</button>
<button on-click='@.fire("func2", event)'>Click 2</button>
</script>

Both buttons callback their respective ractive.on(...) callbacks. But the event object passed is apparently different in the two cases, differing in the event.name property. The first button gets event.name equal to 'func1' (which I expected.) The second button gets 'click', which I didn't expect.

func1 produces this console.log:

func1 Object {node: button, original: MouseEvent, name: "func1", proxy: Element, ractive: Ractive}

func2 produces this console.log:

click Object {node: button, original: MouseEvent, name: "click", proxy: Element, ractive: Ractive}

So...my questions:

A. Are both illustrated uses of on-click supported going forward? B. Are they both working correctly? C. If they are working correctly, why is the event.name different across the two cases?

evs-chris commented 7 years ago

There are some normalization issues with plain fire vs proxied fire. With the proxy event, ractive knows that the argument will be an appropriate event object and will thus set the name on it. With ractive.fire, you can pass whatever args you want to, you just happen to be passing an event object. I'm actually working on a PR for edge right now that addresses that and #2810.

A. Yes. The proxy, as it stands now, is usually more useful for moving component events, though. B. and C. Yes. The name change is actually the only difference between the two.

Jeff17Robbins commented 7 years ago

I almost understand this. :-) When you said

With ractive.fire, you can pass whatever args you want to, you just happen to be passing an event object.

I mostly get that, but somehow Ractive constructed that event object for me, and knew it was in an on-click situation enough to set its event.name to 'click'. I'm still puzzled how the code in proxy event case knows to put the user string 'func1' in the event.name, but in this case the event object is created without that knowledge.

evs-chris commented 7 years ago

Maybe I can string together a coherent explanation... 😄

When you add on-click="..." to an element, ractive will subscribe an event listener (node.addEventListener('click', specialInternalHandler)) to the node when the on-click directive is rendered. It will also remove it when unrendered, hence the working conditional event directive.

When the special handler is called, ractive creates a sort of meta event containing a few relevant bits of information, like the name of the handler on-${nameHere}, the node of the element that contains the event directive (effectively DOMEvent.currentTarget), and the actual DOMEvent as passed to the listener callback. I should note that you can subscribe multiple events to trigger one handler by separating them with - e.g. on-click-touchend-foo-blur, which will create three DOM listeners (click, touchend, and blur) and a custom listener for foo.

That meta event is what is actually passed to whatever is in the ... portion of the on-click directive. What is done with the meta event depends on the .... If it's a proxy event e.g. on-click="go", then ractive knows that you want to fire an event named go using the generated meta event e.g. meta.name = 'go'; ractive.fire('go', meta). It's slightly more complicated than that, but that's the gist of it.

If the ... portion is an expression, then ractive will handle subbing in the special event reference, if there is one, and then evaluate the expression. If you happen to put @.fire('go', event) in there, ractive doesn't really know that you're proxying the event, because it doesn't try to guess what the expressions should do. That being the case, the meta event keeps the original name that it was given.

Jeff17Robbins commented 7 years ago

Quite coherent. Thanks! So, at this point we have two endpoints on the spectrum from specific to general based on what is on the right-hand side of the equals sign of on-${nameHere}:

1. proxy event:  e.g. on-click="go" 
.
.
.

n. expression: e.g. on-click='@this.fire("go", event)'

Which brings me back to a post of yours I didn't have enough context to respond to until now, which I'm now interpreting as an idea for functionality in between these two endpoint:

Thinking about a proxy event replacement construct, how about something like where the event is automatically injected into a call to fire. Other names that look promising are fire-click, when-click, and on-click-fire, so fire-${eventNames}, when-${eventNames}, and on-${eventNames}-fire.

In other words, if I'm following you correctly, you are saying we could have 3 choices, where the middle choice provides syntactic sugar on top of @this.fire(...) to bring the syntax closer to the simpler proxy event?

1. proxy event (simple):  e.g. on-click="go"
2. proxy event (complex): e.g.  on-click-fire="a.b ? 'func1' : 'func2'"
3. expression: e.g. on-click='@this.fire("go", event)'

In 2, I'm assuming that in case func1, that event handler will get called with a meta event argument, and event.name is equal to 'func1'? If those assumptions are correct, I like the idea of having something like this.

I'm not super happy with needing another magic directive, though.

I guess my first question is to ask "why"? Why can't both 1 and 2 be introduced with on-click=? Isn't "go" somehow recognizable as the limiting case of something to wrap with @this.fire()? Or would it need to (awkwardly) be "'go'" (with inner quotes) to be so recognized?

If both 1 and 2 cannot be simply on-click=, then my second question is to ask if the marker could be on the right-hand side instead of the left-hand side. For example (I'll hate myself for suggesting this!):

2.  proxy event (complex): e.g.  on-click=${"a.b ? 'func1' : 'func2'"}

The RETURN of the computed expression syntax?!? Since that special marker ${...} used by computeds seems to serve a similar purpose here? Or is this a misplaced analogy?

evs-chris commented 7 years ago

That actually brings about an obvious solution that I'm not sure why I haven't considered yet: the event directive could evaluate the expression (which always returns an array if it doesn't throw), and if the first item is a string, it could proxy fire it along with the rest of the results in the list as args. That would make these equivalent: on-click="@.fire('foo', 42)" on-click="'foo', 42". The nested quotes aren't great, but as of a week or two ago, there is template string support and backticks aren't usable as attributes quotes. That gets you on-click=`foo`, 42 if you're cool with being a savage that doesn't quote attributes 😄

I think I have a solution for addressing the need to pass event or @context everywhere, but I'll move that part of the discussion to the relevant PR #2853