marko-js-archive / marko-widgets

[LEGACY] Module to support binding of behavior to rendered UI components rendered on the server or client
http://v3.markojs.com/docs/marko-widgets/
MIT License
141 stars 40 forks source link

custom dynamic tag breaking on widget reference in non-widgets #126

Closed yomed closed 8 years ago

yomed commented 8 years ago

I currently have a custom tag called <dynamic> which can be used to dynamically load other components. The components can be templates, renderers, or just widgets. So the template for this tag looks like this:

<if test="data.type === 'widgetWithoutId' || data.type === 'renderer'">
    <invoke function="data.renderer(data.model, out)" />
</if>
<else-if test="data.type === 'template'">
    <include template="${data.template}" entry="$data.model" />
</else-if>
<else-if test="data.type === 'widgetWithId'">
    <invoke function="data.renderer(data.model, out)" w-id="dynamic" />
</else-if>

This compiles to (just render function shown):

  return function render(data, out) {
    var widget = __getCurrentWidget(out);

    if (data.type === 'widgetWithoutId' || data.type === 'renderer') {
      data.renderer(data.model, out);

    }
    else if (data.type === 'template') {
      __helpers.i(out, __loadTemplate(data.template, require), {"entry": data.model});
    }
    else if (data.type === 'widgetWithId') {
      __widgetArgs(out, widget.id, "dynamic");
      data.renderer(data.model, out);

      _cleanupWidgetArgs(out);
    }
  };

If there is currently no parent widget, this fails at var widget = __getCurrentWidget(out); with Error: No widget found at getCurrentWidget. If there is a parent widget somewhere in out, then this seems to work just fine. However, I've now run into a use case where I don't actually have a widget at this point.

Even though the reference to widget is needed for the third case here (in widget.id), it is not needed for the rest of my template. Could __getCurrentWidget(out); be moved closer to where it is used here? Or am I missing something...?

patrick-steele-idem commented 8 years ago

@yomed that's an interesting use case you have there... It doesn't make sense to move __getCurrentWidget(out) closer because the widget reference could be very earily needed in multiple places (for example, if multiple tags have a w-id attribute). It's kind of a hack, but I recommend the following workaround:

Move out the content of the last if into a separate template named ./optional-widget.marko:

<invoke function="data.renderer(data.model, out)" w-id="dynamic" />

And then update your main template accordingly:

<if test="data.type === 'widgetWithoutId' || data.type === 'renderer'">
    <invoke function="data.renderer(data.model, out)" />
</if>
<else-if test="data.type === 'template'">
    <include template="${data.template}" entry="$data.model" />
</else-if>
<else-if test="data.type === 'widgetWithId'">
    <include template="./optional-widget.marko" renderer="${data.renderer}" model="${data.model}" />
</else-if>

I don't think it makes sense to change a Marko Widgets due to this edge case, but let me know if you disagree.

Let me know if the workaround works for you.


On a related note, here's what the simplified code would look like in Marko v3 just as a heads up (the migration tool will make the changes automatically):

<invoke data.renderer(data.model, out) w-id="dynamic" />
<if(data.type === 'widgetWithoutId' || data.type === 'renderer')>
    <invoke function="data.renderer(data.model, out)" />
</if>
<else-if(data.type === 'template')>
    <include template="${data.template}" entry="$data.model" />
</else-if>
<else-if(data.type === 'widgetWithId')>
    <include("./optional-widget.marko") renderer=data.renderer model=data.model />
</else-if>
yomed commented 8 years ago

Ah thanks! The workaround works great - I guess there is some additional overhead in breaking into yet another tag, but it should be pretty minimal since the main use case of the dynamic tag is using non-widgets anyway. Since the workaround is fairly simple, I agree no need to patch marko widgets for an edge case like this.

The v3 syntax looks great, much more natural. Will definitely pick it up shortly in my side projects. Might take a little while for ebay to catch on though...