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

Allow users of components to receive data in the body of a custom tag #656

Closed
mlrawlings opened this issue Apr 6, 2017 · 6 comments
Closed
Labels
proposal status:needs review Needs to be followed up on type:feature A feature request

Comments

@mlrawlings
Copy link
Member

mlrawlings commented Apr 6, 2017

New Feature

Description

list.marko

<ul>
    <li for(itemData in input.items)>
        <include(input.item, itemData)/>
    </li>
</ul>

page.marko

<list items=input.items>
    <@item(data)>
        ${input.name}
    </@item>
</list>

Open Questions

Typically, <tag(data)> has been used to pass data to the tag, not receive data. Is this a good pattern?

@patrick-steele-idem
Copy link
Contributor

I think it would be confusing to use the parens to define variables for the renderBody() function. I'll throw out something that might be a terrible idea:

<list items=input.items>
    <@item renderBody(data)>
        ${data.name}
    </@item>
</list>

I'm not really a fan of this either, because technically it becomes function renderBody(out, data) { }

Maybe the following?:

<list items=input.items>
    <@item input(data)>
        ${data.name}
    </@item>
</list>

With destructuring?:

<list items=input.items>
    <@item input({name})>
        ${data.name}
    </@item>
</list>

@patrick-steele-idem
Copy link
Contributor

After giving it some more thought, it only makes sense to declare additional parameters for a renderBody(out) function if the the callee of the provided renderBody function is going to actually provide additional data as arguments (e.g. input.renderBody(out, item)). For that reason, I think it is the job for the UI component to force the user to declare the variable or possibly provide a default variable name (with the caveat that will be confusing to a user where the variable came from). For example. The <list> component might have the following src/components/list/marko-tag.json:

{
  "<item>": {
    "var": { "nameFromAttribute": "var" }
  }
}

With this tag definition. The user would just need to do the following:

<list items=input.items>
    <@item var="theItem">
        ${theItem.name}
    </@item>
</list>

If the user didn't provide the var attribute then the user would get the following compilation error with the current implementation:

Attribute "var" is required

I think this is the best approach, but I think the following would be the next best alternative:

renderBody() directive

By default, Marko generates adds a renderBody(out) method to the input object if a UI component has nested body content. If the user would like to control that function then they could use the renderBody() directive (as an attribute on the tag):

<list items=input.items>
    <@item renderBody(theItem)>
        ${theItem.name}
    </@item>
</list>

The renderBody() directive could be documented as follows:

When a tag has nested content Marko will automatically add a renderBody(out) method to the input object passed to the target tag renderer. When the input.renderBody(out) method is invoked by the receiving component then the nested body content will be rendered. If you would like to allow a tag to receive additional content then you can use the renderBody(param1, param2, ...) directive as an attribute to declare additional parameters. NOTE: The first parameter of the renderBody method is always out


Thoughts?

@cameronbraid
Copy link
Contributor

cameronbraid commented Sep 19, 2017

I would like to see something like this... I use freemarker server side with a similar feature : http://freemarker.org/docs/ref_directive_macro.html#autoid_114

I 'hack' it in marko like this :

<list items=input.items>
  $ var item = arguments[1].item
  <div.item>${item.name}</div>
</list>

list.marko:
<div.list>
  <for (item in input.items)>
      <include(input) item=item/>
  </for>
</div>

@mlrawlings mlrawlings added proposal type:feature A feature request labels Sep 29, 2017
@patrick-steele-idem patrick-steele-idem added the status:needs review Needs to be followed up on label Oct 3, 2017
@patrick-steele-idem
Copy link
Contributor

patrick-steele-idem commented Oct 3, 2017

Here's another proposal that introduces a new syntax for declaring the renderBody() function:

<await(userPromise)> (user) =>
  Hello ${user.name}!
</await>

// Destructuring would also be allowed:
<await(userPromise)> ({ name }) =>
  Hello ${name}!
</await>

// A one-liner would also be supported:
<await(userPromise)> (user) => Hello ${user.name}!</await>

// A new line should also probably be allowed:
<await(userPromise)>
  (user) => Hello ${user.name}!
</await>

// Or, if you don't need to receive data :)
<await(userPromise)>Hello!</await>

Here's another example that makes use of an alternative to HOCs for watching the scroll position (based on what is described in "I’m Breaking up with Higher Order Components."):

<div>
  <scroll-watch> (x,y) => 
    <show-pos x=x y=y />
  </scroll-watch>
</div>

NOTE: out would always be added as the first parameter for consistency. Technically, the out is not needed if the body is rendered synchronously, but since it may be needed I think we should always add it for consistency. Therefore, the <scroll-watch> component would render its body using the following code:

let { x, y } = this.state;
this.input.renderBody(out, x, y);

Here's another example based on the list rendering use case described in the original comment for this GitHub issue:

<list items=input.items>
    <@item> (item) =>
        ${item.name}
    </@item>
</list>

Pros:

  • We do not have to change how arguments are already used (i.e.: <TAG_NAME(argument)>)
  • Minimal boilerplate
  • renderBody() is not introduced to component consumers (only component authors)
  • Mirrors arrow functions in JavaScript
  • Compatible with current Marko (non-breaking change)
  • Very consistent: Attributes and arguments are used for providing input to another component while (...) => would be used to receive input from another component

Cons:

  • out should automatically be added as a the first parameter for consistency
  • It's a little odd that the renderBody() function is declared outside the start tag
  • There's a very, very small chance this could be a breaking change if someone happened to use <TAG_NAME>\s*(...) => for whatever reason
  • Questionable benefits with the <for> tag:
<ul>
  <for(colors)> (color) =>
    <li>${color}</li>
  </for>
</ul>

@patrick-steele-idem patrick-steele-idem changed the title allow @tags to receive data from <include> calls Allow users of components to receive data in the body of a custom tag Oct 3, 2017
@cameronbraid
Copy link
Contributor

cameronbraid commented Jan 24, 2018

I prefer to have the argument names defined inside the tag

Anyone have an idea about other ways to do it ?

What about using a pipe to delimit the input args from the output arg names

for example :

<tagName inputArg1=inputValue1 | ourputVar1>
  ${outputVar1}
</tagName>

<ul>
  <for(colors) | color>
    <li>${color}</li>
  </for>
</ul>

<list items=input.items>
    <@item | item>
        ${item.name}
    </@item>
</list>

Could use brackets around the output parameter names if there are more than 1

<forEachEntryInMap map=map | (key, value) > 
  ${key} ${value}
</forEachEntryInMap>

Food for thought

@mlrawlings
Copy link
Member Author

Implemented in #1076

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposal status:needs review Needs to be followed up on type:feature A feature request
Projects
None yet
Development

No branches or pull requests

3 participants