Adding dynamic partials, discussed in #54

ghost commented 2 years ago

The idea is to allow to dynamically load partials.

Brief example: Data:

{
  content: "mainpage",
  world: "there"
}

Templates:

{{!template.mustache}}
{{*content}}

{{!mainpage.mustache}}
Hello {{world}}!

Result:

Hello there!

ghost commented 2 years ago

I would like to change your proposal a bit in the opposite direction: keep the examples restricted to partials (or only add dynamic parents), and make the overview at the top more agnostic on whether other tags should support dynamic names as well. This follows the sort-of consensus that started to emerge from https://github.com/mustache/spec/pull/134#issuecomment-987564114 onwards. I also replaced "asterisk operator" by "dynamic partial" in most places, because it seems more accurate. I recognize that you adapted this from "greater-than operator" as it appears in the partials spec, but I think that was already a bit of a misnomer at the time, since the tag as a whole is doing the behavior of a partial, not just the > sigil. In this case, attributing the behavior to the asterisk operator is a bit less accurate still, since it is part of the content of the tag rather than the sigil. The sigil is still >.

Yes, that makes more sense, I agree to the changes you made.
I worked towards defining a more generic "asterisk operator" because it felt more consistent to me and I wasn't sure on how to explain in a spec this new feature.
Now I committed the changes you made and removed the test involving basic interpolation and overall the spec seems to be okay, great work!
So what's left is to review these changes one last time, run the tests and squash all these commits

ghost commented 2 years ago

(I'm reposting the I wrote comment here because somehow it doesn't show up.)

After I read your previous comments I was also thinking of this exact situation.
I'm also not a fan of random spaces placed into the tags, but I think that it would be more consistent if we allow this, because it feels more consistent (although very ugly) and because I wouldn't know how to define the spec otherwise.
To be more clear: I don't really see the dynamic names as analogous to the dotted names when it comes to spacing, because the asterisk is placed before the names.
Should the asterisk stick to the > sigil or should it stick to the name?
Should we take in consideration this test in the partials spec?

  # Whitespace Insensitivity

  - name: Padding Whitespace
    desc: Superfluous in-tag whitespace should be ignored.
    data: { boolean: true }
    template: "|{{> partial }}|"
    partials: { partial: "[]" }
    expected: '|[]|'

I think we should either don't mention whitespaces at all in this spec or allow whitespaces everywhere (like in this last example: {{> * dynamic}}) Hopefully in the next major spec release we'll be more consistent about this

jgonggrijp commented 2 years ago

I disagree that whitespace is all-or-nothing. In general, I think there can be sensible reasons to allow whitespace in some places but not in others.

As I see it, the asterisk is part of the name. That means that for consistency with other tags, whitespace should be allowed between the > and the *, although we could choose not to test this in the spec.

I think that it is entirely a matter of taste whether we also allow a space between the * and the rest of the name. I personally find it ugly, but I could live with allowing it.

The one thing I would want to avoid, is to have a test that endorses a space between the * and the name, but no test that endorses a space between the > and the *. In that sense, I think the asterisk should "stick to the name", as you put it.

Maybe the choice becomes a bit more apparent if we consider linebreaks instead of spaces. Most specs allow linebreaks in these places, but not elsewhere (taking partial tags as an example):

{{>
partial
}}

For dynamic names, taking dynamic partials as an example, the question basically becomes which linebreak should not be there (if any):

{{>
*
key.subkey
}}

I think this is fine:

{{>
*key.subkey
}}

while I find this awkward:

{{>*
key.subkey
}}

ghost commented 2 years ago

I think that it is entirely a matter of taste whether we also allow a space between the * and the rest of the name. I personally find it ugly, but I could live with allowing it.

Yeah, it's definitely a matter of taste.

The one thing I would want to avoid, is to have a test that endorses a space between the and the name, but no test that endorses a space between the > and the . In that sense, I think the asterisk should "stick to the name", as you put it.

I don't find having {{>* x}} awkward because I remember reading somewhere a comment proposing >* as a single sigil rather than a combination of two features.
Considering that this spec is mainly about the deference operator combined with the partial sigil I thought that maybe there are some mustache implementations who will find easier just to treat >* as a single sigil? Maybe we should discourage this 'hacky' solution by making {{>* d}} fail and {{> *d}} succeed... I would be pretty fine with that.

I just noticed that my implementation also does fail this ({{>* d}}) test.
So yeah... I don't know
Should we hear another developer?

I here propose to disallow every newline/whitespace for the next major version xD

jgonggrijp commented 2 years ago

Considering that this spec is mainly about the deference operator combined with the partial sigil I thought that maybe there are some mustache implementations who will find easier just to treat >* as a single sigil?

Yes, it is conceivable that some implementers could approach it that way.

Maybe we should discourage this 'hacky' solution by making {{>* d}} fail and {{> *d}} succeed... I would be pretty fine with that.

In my opinion, it is more important to keep reasonable implementations within reach (and to not break semantics for existing users), than to discourage hacky, unreasonable or "distasteful" implementations. I think we should only make {{>* d}} fail if we believe that the notation is seriously wrong by itself.

I personally feel that the notation is somewhat wrong, but maybe not wrong enough to explicitly forbid it.

Should we hear another developer?

Definitely. I would be really interested in @gasche's and @bobthecow's insights!

ghost commented 2 years ago

Good test @nminet . I'm also in favour of giving the precedence to the asterisk operator, in contrary to what @bobthecow proposed Here ({{*foo.bar.baz}} resolves as {{ dereference(foo.bar.baz) }}) So I approve this, thank you for writing the test. Let's see what other people think about this

ghost commented 2 years ago

Although if we interpret {{ *foo.bar.baz }} as {{ dereference(foo).bar.baz }} there's no way then to get {{ dereference(foo.bar.baz) }} ...unless we introduce parenthesis? That would suck a lot and I think we all agree on this, on the fact that we shouldn't ever implement parenthesis into the tags.
The other way around ( getting {{ dereference(foo).bar.baz }} while having {{ *foo.bar.baz }} as {{ dereference(foo.bar.baz) }} ) is kinda tricky for who didn't implement the pipe | operator ( we would have {{ *foo | bar.baz }}? ), which also isn't specced yet. Edit: I'm not sure if the pipe operator is correct in this case.. I don't know.

So if someone needs to have {{ dereference(foo.bar.baz) }} my take would be pretty much like "refactor your context, not my problem ¯_(ツ)_/¯".. I mean if we want to keep mustache simple we won't meet the needs of everyone, but I think that's fine.. a good deal.. considering that refactoring the context shouldn't be an hard requests for developers.

To me having {{ *foo.bar.baz }} as {{ dereference(foo).bar.baz }} seems more reasonable.
But if we go for the way bobthecow proposed I think we should also spec the pipe operator | with this spec. In that way I'd be fine with the result

I wonder how did @jgonggrijp implement {{ *foo.bar.baz }}, please let us know.

jgonggrijp commented 2 years ago

@nminet

The impact of dotted name on context is not obvious suggest add test

  - name: Dynamic names
    desc: Dotted resolution should push contexts.
    data:
      dynamic: 'obj'
      obj: { sub: { partial: 'name' }, value: 'obj1' }
      value: 'root'
    template: "[{{> *dynamic.sub.partial }}]"
    partials:
      name: "{{value}}"
    expected: '[obj1]'

Oh please, no! The template in that test has only a single asterisk, but it is taking effect three times with different semantics on each occasion.

*dynamic is dereferenced in the context and then resolved in the context, producing an object as the next lookup frame for name resolution. This is new behavior that has not been discussed anywhere before, as far as I can tell.
*(obj.sub.partial) is dereferenced in the context and the resolved as a partial, producing a string naming the partial to be interpolated. This is the behavior that has been discussed so far.
The name partial is evaluated with obj as the top context frame due to a side effect of 1.

To summarize, this test is suggesting that {{>*foo.bar.baz}} should act like

{{# {{foo}} }} {{> {{bar.baz}} }} {{/ {{foo}} }}

if such notation were valid.

Having a single operator that acts twice is akin to -3+6 evaluating to -3 (i.e. -(-3+6)). Having a single operator that acts twice in two different ways is like -3+6 evaluating to -12 (i.e. -(3!+6)). Really, let's not go there, let alone to operators acting three times.

@anomal00us

Although if we interpret {{ *foo.bar.baz }} as {{ dereference(foo).bar.baz }} there's no way then to get {{ dereference(foo.bar.baz) }} ...unless we introduce parenthesis?

Right, or operators that act more than once as I described above. Which would be worse.

That would suck a lot and I think we all agree on this, on the fact that we shouldn't ever implement parenthesis into the tags. The other way around ( getting {{ dereference(foo).bar.baz }} while having {{ *foo.bar.baz }} as {{ dereference(foo.bar.baz) }} ) is kinda tricky...

No it isn't. This is really easy to realize by refactoring the context. It is the same reason why I believe that dynamic Interpolation tags would be pointless. For a simple example, let's say you have this context and you want to dereference(foo).bar.baz:

foo: fuz
fuz:
    bar:
        baz: 'value'

Refactor the context, and now you can simply do foo.bar.baz without any dereference:

foo:
    bar:
        baz: 'value'

... for who didn't implement the pipe | operator ( we would have {{ *foo | bar.baz }}? ), which also isn't specced yet. Edit: I'm not sure if the pipe operator is correct in this case.. I don't know.

I don't know how the pipe operator is supposed to work, but as I illustrated above, I think dereference(foo).bar.baz is not a very convincing use case in the first place.

So if someone needs to have {{ dereference(foo.bar.baz) }} my take would be pretty much like "refactor your context, not my problem ¯(ツ)/¯".. I mean if we want to keep mustache simple we won't meet the needs of everyone, but I think that's fine.. a good deal.. considering that refactoring the context shouldn't be an hard requests for developers.

I guess that technically, you are right that if you have the following context and you want to dereference(foo.bar.baz):

foo:
    bar:
        baz: 'value'

you could rewrite the context to the following and do dereference(baz) instead:

foo:
    bar:
        baz: 'value'
baz: 'value'

However, the nice thing about dereference(foo.bar.baz) is that it is simply a behavior that we already know from Interpolation tags. As @bobthecow proposed it, {{>*foo.bar.baz}} is equivalent to {{> {{foo.bar.baz}} }}, if such notation were allowed. Just compositon of familiar behaviors. Isn't that much more elegant (and much easier to implement)?

To me having {{ *foo.bar.baz }} as {{ dereference(foo).bar.baz }} seems more reasonable. But if we go for the way bobthecow proposed I think we should also spec the pipe operator | with this spec. In that way I'd be fine with the result

Can we please keep it simple and not pile even more operators or semantics on top.

I wonder how did @jgonggrijp implement {{ *foo.bar.baz }}, please let us know.

To be precise, I did not implement {{ *foo.bar.baz }}. I did implement {{> *foo.bar.baz }} and {{< *foo.bar.baz }}, as follows:

Resolve foo.bar.baz in the current context, in the same way {{foo.bar.baz}} would be resolved (but not interpolated). For illustration, assume the result is the string "foobar".
Interpolate {{>foobar}} (or {{<foobar}}{{/foobar}}) as usual.

As I mentioned before, this is exactly like {{> {{foo.bar.baz}} }}, if such notation were allowed. You can read the fine details here, here, here, here and here.

nminet commented 2 years ago

@jgonggrijp, @anomal00us

The interpretation of *x.y.z as "deref(x.y.z) with no context change" makes more sense to me and is in fact the current implementation here - using the asterisk as a sigil rather than a name qualifier, which by the way remains my preferred option.

However it is not what the header description implies, and if dotted name are explicitely allowed, the effect should be specified. The test proposition was just trying to clarify the intent behind the text, as the processing is indeed very irregular.

Anyway we also need to clarify if {{> *dynamic}} pushes the context that resolves 'dynamic', or leaves the current context unchanged (preferred option). Some more tests to think about :-)

ghost commented 2 years ago

@jgonggrijp

No it isn't. This is really easy to realize by refactoring the context. It is the same reason why I believe that dynamic Interpolation tags would be pointless. For a simple example, let's say you have this context and you want to dereference(foo).bar.baz:

I don't know how the pipe operator is supposed to work, but as I illustrated above, I think dereference(foo).bar.baz is not a very convincing use case in the first place.

The nice thing about dereference(foo.bar.baz) is that it is simply a behavior that we already know from Interpolation tags.

Okay it makes sense, you have convinced me.

@nminet

However it is not what the header description implies, and if dotted name are explicitely allowed, the effect should be specified. The test proposition was just trying to clarify the intent behind the text, as the processing is indeed very irregular.

I wrote these new two tests, what do you think?

  - name: Dotted Names
    desc: The dynamic partial should operate within the current context.
    data: { text: 'Hello, world!', foo: { bar: { baz: 'partial' } } }
    template: '"{{>*foo.bar.baz}}"'
    partials: { partial: '*{{text}}*' }
    expected: '"*Hello, world!*"'

  - name: Dotted Names - Failed Lookup
    desc: The dynamic partial should operate within the current context.
    data:
      text: 'Hello, world!'
      foo: 'test'
      test:
        bar:
          baz: 'partial'
    template: '"{{>*foo.bar.baz}}"'
    partials: { partial: '*{{text}}*' }
    expected: '""'

Anyway we also need to clarify if {{> *dynamic}} pushes the context that resolves 'dynamic', or leaves the current context unchanged (preferred option).

I don't understand what do you mean, anyway I think that the context should be immutable when rendering the template.. I think that currently there isn't any tag which modifies the context.

gasche commented 2 years ago

The question I think is whether `{{>* foo.bar }} should be equivalent to {{# foo}} {{>* bar}} {{/ foo}}, where the other values in foo can be named directly from the expansion of {{>* bar}}. (Note: this would then naturally suggest doing something, ahem, interesting, if foo refers to a list/array of objects rather than a single object.) @jgonggrijp is suggesting to not take this interpretation.

nminet commented 2 years ago

@jgonggrijp

The nice thing about dereference(foo.bar.baz) is that it is simply a behavior that we already know from Interpolation tags.

fully agree.

@anomal00us

I don't understand what do you mean, anyway I think that the context should be immutable when rendering the template.. I think that currently there isn't any tag which modifies the context.

with dotted name the partial name may be found by going up and down the stack (not changing the contents but changing the location)

here-below not sure if output should be 'section1' or 'section2' or nothing.

  - name: Dynamic names
    desc: Context tracking?.
    data:
      section1: { value: 'section1' }
      section2: { dynamic: 'partial', value: 'section2' }
    template: "{{#section1}}{{>*section2.dynamic}}{{/section1}}"
    partials:
      partial: "{{value}}"
    expected: '???'

would go for 'section1' (finding partial name having no effect on context)

gasche commented 2 years ago

For me the meaning of {{>* longident }} is to resolve longident in the current scope to a string foo, and then behave as {{> foo}}. This naturally suggests section1 as a result in your test.

ghost commented 2 years ago

@jgonggrijp is suggesting to not take this interpretation.

I would also not rather take this implementation.

@nminet ooh I got it. Well that's interesting, yeah I'd also go for section1 in that test.

jgonggrijp commented 2 years ago

@nminet

The interpretation of *x.y.z as "deref(x.y.z) with no context change" makes more sense to me and is in fact the current implementation here - using the asterisk as a sigil rather than a name qualifier, which by the way remains my preferred option.

Thanks for sharing that. I can relate to preferring * as a sigil, having preferred this myself initially.

(By the way, the link is dead. "kostache" should be "kotlin-mustache".)

However it is not what the header description implies, and if dotted name are explicitely allowed, the effect should be specified. The test proposition was just trying to clarify the intent behind the text, as the processing is indeed very irregular.

Thanks for clarifying that. Maybe this should (also) be addressed by refining the overview text? Would you have a suggestion?

@anomal00us

I wrote these new two tests, what do you think?

  - name: Dotted Names
    desc: The dynamic partial should operate within the current context.
    data: { text: 'Hello, world!', foo: { bar: { baz: 'partial' } } }
    template: '"{{>*foo.bar.baz}}"'
    partials: { partial: '*{{text}}*' }
    expected: '"*Hello, world!*"'

  - name: Dotted Names - Failed Lookup
    desc: The dynamic partial should operate within the current context.
    data:
      text: 'Hello, world!'
      foo: 'test'
      test:
        bar:
          baz: 'partial'
    template: '"{{>*foo.bar.baz}}"'
    partials: { partial: '*{{text}}*' }
    expected: '""'

I think such a pair could be useful, but I would like to replace the second test by the following:

  - name: Dotted Names - Failed Lookup
    desc: The dynamic partial should operate within the current context.
    data:
      foo:
        text: 'Hello, world!'
        bar:
          baz: 'partial'
    template: '"{{>*foo.bar.baz}}"'
    partials: { partial: '*{{text}}*' }
    expected: '"**"'

@nminet

  - name: Dynamic names
    desc: Context tracking?.
    data:
      section1: { value: 'section1' }
      section2: { dynamic: 'partial', value: 'section2' }
    template: "{{#section1}}{{>*section2.dynamic}}{{/section1}}"
    partials:
      partial: "{{value}}"
    expected: '???'

Brilliant. I would like to have this test in the spec, too. I agree with everyone so far that section1 is the right interpretation. I suggest Dotted names - context stacking as the name and Dotted names should not push a new frame on the context stack as the description.

@gasche Are the spec and this discussion moving in a direction that you like, so far?

ghost commented 2 years ago

I think such a pair could be useful, but I would like to replace the second test by the following:

@jgonggrijp I think we should keep also my second test as it emphasizes the fact that {{*foo.bar.baz}} is not {{deref(foo).bar.baz}}.

Your test made me question a few more edge cases:

      - name: Dynamic names
        desc: Context tracking 2?.
        data:
          value: 'test'
          section1: { value: 'section1' }
          section2: { dynamic: 'partial', value: 'section2' }
        template: "{{#section1}}{{>*section2.dynamic}}{{/section1}}"
        partials:
          partial: "{{value}}"
        expected: '???'

      - name: Dynamic names
        desc: Context tracking 3?.
        data:
          value: 'test'
          section1: [ 1, 2 ]
          section2: { dynamic: 'partial', value: 'section2' }
        template: "{{#section1}}{{>*section2.dynamic}}{{/section1}}"
        partials:
          partial: "{{value}}"
        expected: '???'

      - name: Dynamic names
        desc: Context tracking 4?.
        data:
          section1: [ 1, 2 ]
          section2: { dynamic: 'partial', value: 'section2' }
        template: "{{#section1}}{{>*section2.dynamic}}{{/section1}}"
        partials:
          partial: "{{value}}"
        expected: '???'

I'm not sure what should be the output of these tests.. Maybe section1, testtest, and `` empty?
Which of these tests should we include in the spec?
What do you think?

jgonggrijp commented 2 years ago

@jgonggrijp I think we should keep also my second test as it emphasizes the fact that {{*foo.bar.baz}} is not {{deref(foo).bar.baz}}.

Fine by me. I agree it makes this semantic more explicit.

      - name: Dynamic names
        desc: Context tracking 2?.
        data:
          value: 'test'
          section1: { value: 'section1' }
          section2: { dynamic: 'partial', value: 'section2' }
        template: "{{#section1}}{{>*section2.dynamic}}{{/section1}}"
        partials:
          partial: "{{value}}"
        expected: '???'

I agree this one would be section1 again, but I don't think it is very informative to add this if we already have the original test by @nminet.

      - name: Dynamic names
        desc: Context tracking 3?.
        data:
          value: 'test'
          section1: [ 1, 2 ]
          section2: { dynamic: 'partial', value: 'section2' }
        template: "{{#section1}}{{>*section2.dynamic}}{{/section1}}"
        partials:
          partial: "{{value}}"
        expected: '???'

I also agree that this one would be testtest. Useful, maybe.

      - name: Dynamic names
        desc: Context tracking 4?.
        data:
          section1: [ 1, 2 ]
          section2: { dynamic: 'partial', value: 'section2' }
        template: "{{#section1}}{{>*section2.dynamic}}{{/section1}}"
        partials:
          partial: "{{value}}"
        expected: '???'

Again, I agree with you on the expected output, i.e., the empty string. That also means it could easily pass by accident, though. If we include both the original test by @nminet and the "context miss" test that I suggested myself, I don't think this one adds much.

ghost commented 2 years ago

Okay I got it, I will commit these tests soon!

ghost commented 2 years ago

I don't know which names and descriptions I should use for these tests:

  - name: Dynamic Names
    desc: Context tracking.
    data:
      section1: { value: 'section1' }
      section2: { dynamic: 'partial', value: 'section2' }
    template: "{{#section1}}{{>*section2.dynamic}}{{/section1}}"
    partials:
      partial: "{{value}}"
    expected: 'section1'

  - name: Dynamic Names
    desc: Context tracking.
    data:
      value: 'test'
      section1: [ 1, 2 ]
      section2: { dynamic: 'partial', value: 'section2' }
    template: "{{#section1}}{{>*section2.dynamic}}{{/section1}}"
    partials:
      partial: "{{value}}"
    expected: 'testtest'

I think we should add the last test I wrote earlier here because I find it to be more clear/expressive.

jgonggrijp commented 2 years ago

For the first, I previously suggested Dotted names - context stacking as the name and Dotted names should not push a new frame on the context stack as the description.

For the second, you could go with Dotted names - context stacking under repetition and maybe just the same description as the first.

Please submit the next iteration however you think is best; we will simply review again.

nminet commented 2 years ago

(By the way, the link is dead. "kostache" should be "kotlin-mustache".)

My bad - repo was still private I was planning to complete kotlin-multiplatform first but ok - as-is.

ghost commented 2 years ago

Okay I've pushed the changes, thank you for the help

ghost commented 2 years ago

I still feel this space doesn't belong here, because the test is about indentation rather than internal padding. The space can optionally be transplanted to the Padding Whitespace test.

@jgonggrijp So our take for whitespacing would be to not mention it in the tests? This may be the only thing left to think about.
I'm okay with removing it. But I'd also be okay with allowing {{> * dynamic}}

ps. I don't know why but my comments on your reviews get labeled as 'Pending' and sometimes don't show up

ghost commented 2 years ago

I'm removing the space in that case because it isn't the point of that test.
So my question now is, should we add tests for whitespaces within the tag? And if so what do we allow?

jgonggrijp commented 2 years ago

@anomal00us

ps. I don't know why but my comments on your reviews get labeled as 'Pending' and sometimes don't show up

Ah, I think I understand what's happening with your comments. When you add a line comment or reply, you get two submit buttons: "add single comment" or "start/add to review". The latter is chosen if you submit with the keyboard. The latter also means that your comment is put in a queue (hence "pending") that will be published all at once when you finish your review. You can do that in the top right of the "files changed" tab (green button that expands to another comment form). Until you submit the overall review, your pending comments are invisible to other users, and apparently sometimes also to yourself.

(This was not my idea, I discovered it by accident.)

I'm removing the space in that case because it isn't the point of that test. So my question now is, should we add tests for whitespaces within the tag? And if so what do we allow?

You currently test for {{> *dynamic }} in the Padding Whitespace test. @gasche, I've seen you write a space between the asterisk and the name ({{>* dynamic}}). Is that notation important enough to you that you want to spec it? In that case I suggest changing the notation in Padding Whitespace to {{> * dynamic }}.

ghost commented 2 years ago

Alright, I'm done!

ghost commented 2 years ago

Ah, I think I understand what's happening with your comments. When you add a line comment or reply, you get two submit buttons: "add single comment" or "start/add to review". The latter is chosen if you submit with the keyboard. The latter also means that your comment is put in a queue (hence "pending") that will be published all at once when you finish your review. You can do that in the top right of the "files changed" tab (green button that expands to another comment form). Until you submit the overall review, your pending comments are invisible to other users, and apparently sometimes also to yourself.

(This was not my idea, I discovered it by accident.)

ooh I got it! Thank you!

jgonggrijp commented 2 years ago

I retried the spec with my implementation, and everything passed again, except for the test that has a space between the asterisk and the name. I'll bugfix that in my implementation.

jgonggrijp commented 2 years ago

I'll wait for @nminet and @gasche to give an explicit green light, and I'll give @bobthecow a week to reply in case he still wants to do that. Once all of that is over, I'll merge this (I joined @Danappelxx and @spullara as a maintainer).

ghost commented 2 years ago

I'll wait for @nminet and @gasche to give an explicit green light, and I'll give @bobthecow a week to reply in case he still wants to do that. Once all of that is over, I'll merge this (I joined @Danappelxx and @spullara as a maintainer).

Awesomee, I'm glad we've come this far.
It's been a pleasure to work with you!

nminet commented 2 years ago

I'll wait for @nminet and @gasche to give an explicit green light, and I'll give @bobthecow a week to reply in case he still wants to do that. Once all of that is over, I'll merge this (I joined @Danappelxx and @spullara as a maintainer).

ok for me

good to get this done - thanks all :-)

gasche commented 2 years ago

I think the work on the spec is very nice, but I also think that the justification for the feature is not documented very well. Currently it is spread among many different issues with back-and-forth and ongoing evolution of the proposed feature. We get a spec that normalizes a consensual result, and this is great, but what we don't get from this PR is a coherent, synthetic explanation of why the feature is worth it. I think it would be important to write it, to answer questions in the future about "why did they add this to a minimalistic template language?" or "doesn't this serve the same purpose as XXX ?".

Compare this to the RFC process in various communities: the document that evolves during the discussion, and is finally accepted, contains not only a specification of a proposal, but also its context, justification, pros/cons, alternative considered etc.

Currently the spec repository does not contain, as far as I know, justification/rationales for the specced features. I see two ways forward:

the justification could be written as one post here on github (edited until people are satisfied); not ideal but way better than small bits of arguments all over the place
or we could start adding such justifications to the specifications directly, as a new section.

To summarize: the question that needs to be answered in a coherent document is "Why do we want this feature?"

I think that (2) is better than (1), and that at least (1) should be a requirement before merging the spec.

ghost commented 2 years ago

Do you think we should add this last test?

  - name: Basic Behavior - Context Collisions
    desc: The Dynamic Name should resolve within the current context stack first.
    data: { dynamic: 'content', *dynamic: 'wrong' }
    template: '"{{>*dynamic}}"'
    partials: { content: 'Hello, world!', wrong: 'Invisible' }
    expected: '"Hello, world!"'

jgonggrijp commented 2 years ago

@gasche

I see two ways forward:

the justification could be written as one post here on github (edited until people are satisfied); not ideal but way better than small bits of arguments all over the place

or we could start adding such justifications to the specifications directly, as a new section.

To summarize: the question that needs to be answered in a coherent document is "Why do we want this feature?"

Just checking: apart from the fact that the motivation is not yet documented, do you personally feel that the addition of dynamic names is sufficiently justified in the first place? I take "no" for an answer, if that is what you feel.

I agree that a rationale is good to have and I would personally welcome option 2. I don't think it strictly needs to be a separate section; it could also be a paragraph within the overview, or multiple paragraphs if necessary.

@nminet and @anomal00us, would you agree with adding a rationale to the spec?

If so, who is going to write the first version? I would suggest either @anomal00us, who is the main editor of this spec, or @gasche, who proposed including a rationale.

Some possible inputs for the rationale:

https://github.com/mustache/spec/issues/54#issuecomment-985286127 and the following two comments (respectively by @anomal00us, @gasche and again @anomal00us)
This CoffeeScript module by my own writing has a real-world use case. The template on line 33 is a dynamic partial, which expands to one of the templates ESM, AMD or CommonJS on lines 11-25. It is honestly a bit thin, as it would be relatively easy to do the dynamic selection in the programming language in this case, but one could imagine that dynamic partial being buried in a larger template, in which case I think there would be no concise alternative.

@anomal00us

Do you think we should add this last test?

  - name: Basic Behavior - Context Collisions
    desc: The Dynamic Name should resolve within the current context stack first.
    data: { dynamic: 'content', *dynamic: 'wrong' }
    template: '"{{>*dynamic}}"'
    partials: { content: 'Hello, world!', wrong: 'Invisible' }
    expected: '"Hello, world!"'

I would vote in favor. Although I think the name and the description don't fit the test very well. How about "Basic Behavior - Name Resolution" and "The asterisk is not part of the name that will be resolved in the context"?

gasche commented 2 years ago

Just checking: apart from the fact that the motivation is not yet documented, do you personally feel that the addition of dynamic names is sufficiently justified in the first place? I take "no" for an answer, if that is what you feel.

TL;DR: "yes but I'm not sure".

Long answer: I remember being skeptical about the feature at first, then being convinced by the idea that it's painful to do without for some use-cases (although the "use N sections for a N-way if-then-else" approach is okay I think). But that was months ago and I flushed the argument from my medium-term memory (and, as discussed, it is currently hard to reconstruct the discussion after the fact), so I'm not currently fully convinced, but rather I remember that I was convinced in the past, and also I'm inclined to trust the judgment of other people (including you) that have worked on it and are now satisfied.

My suspicion is that:

maybe there wasn't a full, explicit discussion of whether we really want the feature in its current form, we jumped from "discussing whether people like it" to "writing a spec" before a consensus was fully-formed
having a process where we write/maintain a justification as we spec the feature would help keep this question in mind throughout the discussion
but, all this said, it's probably the case that even with this more explicit discussion and this more rigorous (and also more work-consuming) process, we would decide to accept the feature anyway.

ghost commented 2 years ago

@nminet and @anomal00us, would you agree with adding a rationale to the spec?

Yes I would be fine with that. I could try to write a draft for a rationale, although I've never written one.

I would vote in favor. Although I think the name and the description don't fit the test very well. How about "Basic Behavior - Name Resolution" and "The asterisk is not part of the name that will be resolved in the context"?

Yeah that sounds better, I'm always having trouble at finding correct titles and descriptions.

@gasche I tried to explain my reasons in the comment jgonggrijp mentioned earlier
Basically a while ago in a php project I wrote I had templates who were pretty much like this:

<html>
<head><title><?=$title;?></title></head>
<body>
<!--you got the idea, here is a header, which is pretty much the same for all pages-->
<?php include $content; // this is the actual page content, the file `$content` only contains its content ?>
<!--and here is the footer-->
</body>
</html>

So when I tried to move this template from php to a template engine syntax I was having some problems.
I mean to get the template working I had to use lambdas or some hacky but bad looking solutions (which I have somewhat described in that comment).

<html>
<head><title>{{title}}</title></head>
<body>
{{><?=$content;?>}}
</body>
</html>

Things like this.. really a mess..
So it felt pretty natural to me to just have this:

<html>
<head><title>{{title}}</title></head>
<body>
{{>*content}}
</body>
</html>

ghost commented 2 years ago

The rationale I wrote:

  Rationale: this special notation was introduced specifically to allow the
  dynamic loading of partials. Although there are already solutions that allow
  to dynamically load partials, these solutions either require the use
  of optional (and complex) features such as lambdas, or require mechanisms that
  in interpreted programming languages are rather tedious to implement and are
  also rather inefficient, both in terms of space, due to a possible necessary
  overhead of the included partials, and in terms of computational efficiency,
  regarding a possible necessary pre-buffering.

I don't know if that's good, let me know. Also, I placed it at the top of the overview, is that okay?
Edit: I found a typo, I edited it here but I'll wait your reviews before committing.

gasche commented 2 years ago

I don't understand the efficiency argument. Can you show an example of the alternatives you have in mind?

I think the justification would benefit from discussing a concrete example, as we did during the design discussions. Show a particular output (or sketch of output) we could reasonably want, show to express it with the feature, and show that other approaches and workarounds are substantially worse.

ghost commented 2 years ago

Okay I'll write some examples
Let's say I have these templates

{{!mainpage.mustache}}
this is the main page.

{{!infopage.mustache}}
this is the info page.

which should be included into this template:

{{!template.mustache}}
header
{{!content goes here}}
footer

So I get

header
this is the info page.
footer

and

header
this is the main page.
footer

These are the solution for achieving that:

Using pre-buffering

The idea is to inject through php the partial name into the template. So our template will look like something like this:

{{!template.mustache.php}}
header
{{><?=$content;?>}}
footer

and our code will look like something like this:

ob_start();
include "template.mustache.php";
$raw_template = ob_get_clean();
$TemplateEngine->renderRawTemplate($raw_template); // this is the idea

The bad thing about this method is mixing php templating and mustache templating.
It's also slower since we technically render the template twice.

Using Lambdas

I'm not sure about this since I don't have much experience with mustache lambdas, anyway...
In this case the template would look something like this:

{{template.mustache}}
header
{{{content}}}
footer

and the code would look something like this:

$context["content"] = function () { return '{{>' . $content .  '}}'; }
// Then we would have:
$template->render($context);

I'm not really a lambda fan, I prefer to have contexts with static data only, but that's a matter of taste.
The thing I don't like about this is again mixing php code and the mustache templates.. It doesn't look that bad but yet we still have a tag {{>$content}} in the php code.
So: php builds the template -> we render it once injecting the include tag into it (we can optimize this part by trying to evaluate only the lambda tags) -> we then render it fully. Let me know what do you think about this

Using feature detection as @jgonggrijp suggested in issue #54

Here the idea is to have a list of mutual exclusive partials for all the possible partials.
The main template so will become:

{{!template.mustache}}
header
{{>content}}
footer

And we now have to introduce a new template which would be something like this:

{{!content.mustache}}
{{#mainpage}}
  {{>mainpage}}
{{/mainpage}}
{{#infopage}}
  {{>infopage}}
{{/infopage}}

Needless to say that this approach isn't suitable if the list is too long.

Template inheritance, suggested by @gasche in issue #54

The idea is to use template inheritance and load the specific pages instead of the general template.
So our template will be like this:

{{!template.mustache}}
header
{{$content}}
footer

and our pages template will look like this:

{{!mainpage.mustache}}
{{< template }}{{$content}}
{{!mainpage.mustache}}
this is the main page.
{{/content}}{{/base}}

{{!infopage.mustache}}
{{< template }}{{$content}}
this is the info page.
{{/content}}{{/base}}

This approach works, though it brings a overhead to all the pages and binds them to template.mustache

Finally, dynamic names

The solution this spec proposes:

{{!template.mustache}}
header
{{>*content}}
footer

ghost commented 2 years ago

ps. I'm not sure wether pre-buffering is the correct term there but you get the idea.

jgonggrijp commented 2 years ago

Some possible inputs for the rationale:

After my previous post, I realized that I only suggested inputs for demonstrating why dynamic partials are useful. That is only half of the rationale we need. The other half is for the choice to introduce a dereference operator for names, rather than adding a new sigil (which @nminet wrote to prefer just recently). I think that other half of the rationale has two parts:

Dynamic name resolution is conceivably useful not only for partials, but also for parents and potentially for other tags in the future.
A tag with dynamic name resolution behaves exactly like its static counterpart, once the name is resolved. Since the only difference is in the name resolution, it makes sense to have an operator act on the name, analogous to the dot operator, instead of introducing a new sigil.

@gasche

My suspicion is that:

maybe there wasn't a full, explicit discussion of whether we really want the feature in its current form, we jumped from "discussing whether people like it" to "writing a spec" before a consensus was fully-formed

having a process where we write/maintain a justification as we spec the feature would help keep this question in mind throughout the discussion

but, all this said, it's probably the case that even with this more explicit discussion and this more rigorous (and also more work-consuming) process, we would decide to accept the feature anyway.

My suspicion is that you are right on all counts. :-)

@anomal00us

I find the use case, where different pages use the same overall layout with common elements, not convincing enough. Consider a common layout with more than one customization point:

<head></head>
<intro>
    <!-- first customization point -->
</intro>
<sidebar></sidebar>
<content>
    <!-- second customization point -->
</content>
<footer></footer>

Let's say that the main page should render a carousel in the intro section, which brings various parts of the site under the attention of the visitor, while the info page should render a table of contents with a search bar in the intro section. Let's also assume that we already have carousel.mustache and search_toc.mustache for those features, respectively.

My personal inclination, and probably that of many people, would be to use inheritance for this purpose, even if dynamic partials are available:

{{!common.mustache}}
<head></head>
<intro>
    {{$intro}}{{/intro}}
</intro>
<sidebar></sidebar>
<content>
    {{$content}}{{/content}}
</content>
<footer></footer>

{{!mainpage.mustache}}
{{<common}}
    {{$intro}}
        {{>carousel}}
    {{/intro}}
    {{$content}}
        this is the main page.
    {{/content}}
{{/common}}

{{!infopage.mustache}}
{{<common}}
    {{$intro}}
        {{>search_toc}}
    {{/intro}}
    {{$content}}
        this is the info page.
    {{/content}}
{{/common}}

You are right that there is less syntactical overhead in the templates when using dynamic partials instead:

{{!common.mustache}}
<head></head>
<intro>
    {{>*intro}}
</intro>
<sidebar></sidebar>
<content>
    {{>*content}}
</content>
<footer></footer>

{{!mainpage.mustache}}
this is the main page.

{{!infopage.mustache}}
this is the info page.

However, in exchange for the syntactical overhead, inheritance also offers some clear benefits over dynamic partials. Firstly, in the inheritance version, all the information on how to render the main page is encapsulated in mainpage.mustache. In order to render it, I do not need to be aware of common.mustache or carousel.mustache. By contrast, in the dynamic partials version, rendering mainpage.mustache by itself is not useful to me. I need to know that I have to render common.mustache and pass {intro: "carousel", content: "mainpage"} as additional data. I could accidentally mix things up and insert the searchable toc on the main page instead, or forget to fill one of the slots. This knowledge burden increases with the number of customization slots as well as with the number of pages that vary on common.mustache.

Secondly, since mainpage.mustache is a complete template in the inheritance version, I can continue extending it. For example, I could create a blackfriday.mustache which inherits from mainpage.mustache, but which has a big "Order now!" banner in the intro section instead of the usual carousel. Again, the user would not need to be aware of how blackfriday.mustache works in order to use it.

In summary, you trade overhead in the template for overhead on the data preparation side. I'm not saying that using dynamic partials for this purpose is wrong, but I also do not think that this scenario is a clear win for dynamic partials.

If it is just another way to do something that is already possible, but with a different tradeoff, then I personally find that insufficient justification for introducing a new feature to a language. However, I think that polymorphism might be a scenario where dynamic partials really outshine the other options.

The "feature detection" approach I suggested in https://github.com/mustache/spec/issues/54#issuecomment-985100035 (basically the Mustache equivalent of a big if-else tree) was in response to a polymorphism scenario, where a mixed list of items is rendered*. Items have different types and each type requires a different way of rendering.

For that scenario, inheritance is not an option. The other options that you discussed (pre-buffering, lambdas and if-else trees) are available, but (I think) clearly worse than dynamic partials.

Concluding, I would suggest using the polymorphism use case to motivate why dynamic partials are needed. Then you could continue to show how this extends to parents, when the alternative templates have particular customization points in common. You could also still use the common layout scenario to compare the tradeoffs between dynamic partials and inheritance.

*) This was the original scenario motivating the discussion in #54, but the solution proposed at the time was different from what we are discussing here. What we have here is far superior, I think.

ghost commented 2 years ago

However, in exchange for the syntactical overhead, inheritance also offers some clear benefits over dynamic partials. Firstly, in the inheritance version, all the information on how to render the main page is encapsulated in mainpage.mustache. In order to render it, I do not need to be aware of common.mustache or carousel.mustache. By contrast, in the dynamic partials version, rendering mainpage.mustache by itself is not useful to me. I need to know that I have to render common.mustache and pass {intro: "carousel", content: "mainpage"} as additional data. I could accidentally mix things up and insert the searchable toc on the main page instead, or forget to fill one of the slots. This knowledge burden increases with the number of customization slots as well as with the number of pages that vary on common.mustache.

Rather than encapsulation I'd call it binding/coupling.. and that's what I don't like about template inheritance: there would be a problem if I wanted to render the content (mainpage.mustache) inside another common template, let's say common2.mustache.
I know that this may be an edge case, but to me it's a fair reason to use dynamic partials.
We have two different philosophies on this, for me it's a good trade off having context overhead instead of template overhead.

I think that polymorphism might be a scenario where dynamic partials really outshine the other options.

Agreed!

Concluding, I would suggest using the polymorphism use case to motivate why dynamic partials are needed.

Okay so I'll try to edit the rationale I wrote. Should I write an example and relative solutions for the polymorphism problem like I did here?

ghost commented 2 years ago

New rationale text:

  Rationale: this special notation was introduced specifically to allow the
  dynamic loading of partials. The main advantage that this new special notation
  offers is to allow dynamic loading of partials, which is particularly useful
  in cases where there needs to be rendered polymorphic data or in cases where
  there is a child template shared by multiple parent templates; cases which
  otherwise would be possible to render only by using either complex (and
  optional) features such as lambdas, or by using solutions that are inefficient
  both in terms of space and in terms of computational efficiency, such as:
  overloading the template with if blocks or preprocessing the template.

jgonggrijp commented 2 years ago

Okay so I'll try to edit the rationale I wrote. Should I write an example and relative solutions for the polymorphism problem like I did here?

Yes I think that would help. The paragraph you wrote is very condensed, it's easier to understand with examples.

New rationale text:

  Rationale: this special notation was introduced specifically to allow the
  dynamic loading of partials.

I would formulate that as "primarily" rather than "specifically". "Specifically" might seem a bit too exclusive.

  The main advantage that this new special notation
  offers is to allow dynamic loading of partials, which is particularly useful
  in cases where there needs to be rendered polymorphic data or in cases where
  there is a child template shared by multiple parent templates;

The last part, "or in cases where there is a child template shared by multiple parent templates", doesn't sit well with me. I have a hard time imagining how multiple parents can share a child (imagining the opposite is much easier). Also, when considering a dynamic partial and the template that embeds it, it is not obvious to me which should be considered the parent and which the child; this relationship doesn't really seem to apply.

Rather than fixing this part, though, I suggest leaving it out. My impression is that you are referring to the common layout scenario. While I'll acknowledge that dynamic partials can be used for that scenario, I think the polymorphic scenario is sufficient to justify the introduction of dynamic partials.

  cases which
  otherwise would be possible to render only by using either complex (and
  optional) features such as lambdas, or by using solutions that are inefficient
  both in terms of space and in terms of computational efficiency, such as:
  overloading the template with if blocks or preprocessing the template.

I would shorten this to "which would otherwise be possible to render only with solutions that are convoluted, inefficient, or both". And then continue to show this with examples.

bobthecow commented 2 years ago

I'll wait for @nminet and @gasche to give an explicit green light, and I'll give @bobthecow a week to reply in case he still wants to do that.

A bit busy right now, but I'll take you up on that week 😛

jgonggrijp commented 2 years ago

@bobthecow Please don't feel rushed, I didn't intend that week as a deadline but rather as a grace period to give you an opportunity to respond. Put differently, you responded within a week so you already met the "deadline".

We're also still figuring out the rationale, so please take as much time as you need.

bobthecow commented 2 years ago

The current PR spec passes in my Mustache.php implementation (only took ~35 lines of code!)

https://github.com/bobthecow/mustache.php/commit/dc4445b16a663ee595c7ec86e66757c5937591f4

I added support for {{>* foo}} and {{<* foo }}. It also works for any other tag type, with no additional change, if I remove the parse exception 🙂

jgonggrijp commented 2 years ago

Well done, @bobthecow. Does this mean you approve of the spec as it currently stands?

bobthecow commented 2 years ago

A couple of suggestions, nothing major!

ghost commented 2 years ago

Thank you everyone for the reviews, I'll commit some changes soon.

mustache / spec