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

Get helper improvements #5993

Closed
3 of 4 tasks
ErisDS opened this issue Oct 23, 2015 · 6 comments
Closed
3 of 4 tasks

Get helper improvements #5993

ErisDS opened this issue Oct 23, 2015 · 6 comments

Comments

@ErisDS
Copy link
Member

ErisDS commented Oct 23, 2015

Having played with the get helper some more since it got merged, there are some details which need addressing as well as some updates needed to make it work nicely with the filter parameter.

I've kept everything in one issue because I hope that'll help to make it all make sense - the blockParams and data naming stuff is highly confusing.

1. Bugfix: Data is getting merged

The data fetched by the helper is being incorrectly merged with the existing data for the template. This means that in certain cases one posts object will get overwritten by another. And you won't get the data that you expect when inside the {{#get}} helper.

I remember I was attempting to resolve an issue when I added the _.merge() line, but what it was escapes me and I'm very sure it's the wrong thing to do, so this needs to be removed.

2. Addition: Pagination block param

Although I added support for blockParams I only did this for the main result data, and not the pagination data. What this means is that you can 'name' the list of posts output by the get helper like this: {{#get "posts" filter="featured:true" as |featured|}} - the |featured| part is what handlebars calls a block param (see the rfc).

I think it makes sense to be able to do {{#get "posts" filter="featured:true" as |featured pagination|}} which is easy to make work but not currently possible with my implementation. Note that without doing this, access to pagination looks like `{{meta.pagination}} - see the part about data naming inconsistencies in 4.

3. Addition: Get helper path support for filters

The get helper will currently work with any kind of filter query, for example {{#get "posts" filter="featured:true"}} or {{#get "posts" filter="tags:photo"}} which makes it possible to do queries to fetch extra data for your theme. However, there is no way currently to build dynamic queries based on data you've already got:

E.g. {{#get "posts" filter="tags:[{{post.tags}}]"}} might reasonably fetch the posts with the same tags as the current post. Or even {{#get "posts" filter="tags:[{{post.tags}}]+id:-{{post.id}}"}} could fetch posts with the same tags as the current post which also aren't the current post. To make this work, we need to explicitly add path resolution to the get helper, so that it will process handlebars-like strings out of the filter query and grab the requested data in a useful format.

Handlebars has its own path syntax, supporting dot syntax like post.author.name and an array syntax like post.tags.[0].slug. Very similar to this is jsonpath - a library which provides an xPath-like syntax for grabbing data from a JSON object.
JSONPath syntax is very slightly different to handlebars path syntax in that its array syntax doesn't require a dot, perhaps more like you would expect: post.tags[0].slug and it can understand wildcards in arrays, so post.tags[*].slug would return an array containing the slug of each tag associated with post.

It's incredibly powerful, and used with the get helper it makes for powerful tool. I propose that we use JSONPath for parsing paths, with two slight modifications:

  1. we parse out the extra dot used in handlebars array syntax, so that if that is used, it still works
  2. we add two aliases {{post.tags}} for {{post.tags[*].slug}}and {{post.author}} for {{post.author.slug}} to cover common use cases.

4. Discussion: Get helper data naming vs basic theming

In the normal theme world, on a post listing page you do {{#foreach posts}} and on a post page you do {{#post}}. Although the API returns the same plural form for multiple and single items, we specially preprocess the data before the theme gets it to make this more intuitive.

The get helper, currently, doesn't do this preprocessing for you. This results in two inconsistencies:

Inconsistency 1 - posts or post?

Both {{#get "posts" filter="featured:true"}} and {{#get slug="my-post"}} do two different queries to the API - the first is a browse and the latter is a read, so the first returns multiple results and pagination, the second returns a single result. However both result in a new posts (plural) array of object(s), so the way you handle the data inside the {{#get}} helper is the same for both:

{{#get "posts" filter="featured:true"}}
    {{#foreach posts}}{{title}}{{/foreach}}
{{/get}}
{{#get "posts" slug="my-post"}}
    {{#foreach posts}}{{title}}{{/foreach}}
{{/get}}

I'm not sure if it's confusing that the output from the second one is also 'posts' or if it is better that the two are consistent as there is little to tell between them. Note that if you wanted you could name the second one 'post' yourself:

{{#get "posts" slug="my-post" as |post|}}
    {{#post}}{{title}}{{/post}}
{{/get}}

This works because, weirdly {{#foreach posts}}{{title}}{{/foreach}} and {{#posts}}{{title}}{{/posts}} both work although they behave slightly differently.

An alternative would be to duplicate the object with both the plural and single name, but this seems like overkill.

Inconsistency 2 - pagination

When returned to a theme currently, pagination is a top level key so you do {{pagination}}. When returned from the API to the get helper, it is {{meta.pagination}}. I think it unlikely that there will be much need for creating pagination inside of {{#get}} queries, but this could trip people up. We could move it to a top level key, but this would be inconsistent with API documentation.

Providing the second point in this issue is solved, it would be possible to solve this again by giving the data a name yourself:

{{#get "posts" filter="featured:true" as |featured pagination|}}

Ultimately, these are both, I think, quite edge case issues that will trip a few people up. This can be mitigated by writing the get helper documentation to encourage the use of block params to name the data that is returned, so that theme developers have absolute control.

We could also duplicate the data under different keys, or apply the same pre-processing that we do for
themes, the question is - which is least confusing?

I don't think there is an easy answer, so it may make sense to move the {{#get}} helper behind the Public API labs flag for the first release, get some feedback, make any necessary changes and then take the flag off in the following release.

Todo list:

  • Don't merge data
  • Support pagination as a block param
  • Add path resolution for filter strings
  • Consider how to name the output data
@ErisDS ErisDS added the themes label Oct 23, 2015
@ErisDS ErisDS added this to the Public API v1 milestone Oct 23, 2015
ErisDS added a commit to ErisDS/Ghost that referenced this issue Oct 23, 2015
refs TryGhost#5993

- Don't merge the result set with the existing template data.
- If available, return `meta.pagination` as the second blockParam
ErisDS added a commit to ErisDS/Ghost that referenced this issue Oct 23, 2015
refs TryGhost#5993

- deps: [email protected]
- adds `resolvePaths` method
- supports handlebars style arrays with `.[]`
- supports shorthand post.tags and post.author for common usecases
- adds more tests & improves existing ones
@vadimdemedes
Copy link

Regarding posts or post?, I think this behavior would be expected:

// plural for array of posts
{{#get "posts" filter="featured:true"}}
  // iterate and render all posts
  {{#posts}}
    <h1>{{title}}</h1>
  {{/posts}}
{{/get}}

// singular for one post
{{#get "post" slug="my-awesome-post"}}
  <h1>{{title}}</h1>
{{/get}}

So, basically, taking into account singular/plural form of a resource (post, posts) and returning results accordingly. This also eliminates some extra code for singular post rendering, like {{#foreach posts}}{{/foreach}} and as |post|.

@ErisDS ErisDS modified the milestone: Public API v1 Dec 3, 2015
@vislamov
Copy link

Re: Addition: Get helper path support for filters

It also would be nice to support and in the query:

E.g. {{#get "posts" filter="tags:[{{post.tags}}]+tag:new-tag-value+id:-{{post.id}}"}} wich will grab non-current post with the same tags and a new tag.

@ErisDS
Copy link
Member Author

ErisDS commented Dec 14, 2015

@vislamov and support is documented by #6158

@jerrylopez
Copy link

@ErisDS I'm seeing this on the list, but is nesting get helpers something you guys are currently working on?

For example:

{{#get "tags"}}
    {{#foreach tags}}
        <h1>{{name}}</h1>
        {{#get "posts" filter="tags:{{id}}"}}
            <ul class="posts">
            {{#foreach posts}}
                <li class="post"><a href="{{url}}">{{title}}</a></li>
            {{/foreach}}
            </ul>
        {{/get}}
    {{/foreach}}
{{/get}}

@ErisDS
Copy link
Member Author

ErisDS commented May 8, 2016

Hi @jerrylopez, nesting the get helper is not something that's currently in our plans. Not being able to nest it is a fundamental limitation of asynchronous helpers. Asynchronous helpers are something which are added as an extension by express-hbs due to lack of support in handlebars. This means it's entirely a bit of a hack.

The intention of the get helper feature is to give users an easy way to pull additional data into a template but in a deliberately limited way as all of this is being done server side. Nesting get helpers as in your example could cause a page speed issue unless you're heavily caching your blog.

This level of customisation is probably better done client side, using AJAX and the API directly.

@kirrg001 kirrg001 mentioned this issue Jan 17, 2017
14 tasks
@ErisDS
Copy link
Member Author

ErisDS commented Mar 14, 2017

I'm going to close this now. We had one piece of feedback in #7242 which I have taken on board and fixed in #8160. If there is more feedback I'm all ears, but there's no need to have this issue open anymore as I think the current API is working well.

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

No branches or pull requests

4 participants