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

Frontmatter properties replace those in .json files rather than supplementing #147

Closed
philhawksworth opened this issue Jun 24, 2018 · 30 comments

Comments

Projects
None yet
7 participants
@philhawksworth
Copy link

commented Jun 24, 2018

This may be by design, but I had expected that the properties defined in a folder's .json would supplement those defined in a file's front matter.

Currently the frontmatter replaces the properties in a folder's .json file. This is most obvious when dealing with arrays such as tags.

It would be really useful to be able define a tag for every file in a folder so that each file could be added to a collection, but then to be able to add further tags in the frontmatter of each file.

An example of this current behavior is illustrated here:

Source: https://github.com/philhawksworth/test-11ty-tags
Demo: https://peaceful-pare-34afb6.netlify.com/

Current behavior:

  • Only those files in the posts directory without tags in their frontmatter inherit the "post" tag from the posts.json file.
  • The posts collection (listed on the home page) excludes any files with their own tags

Expected (ok, desired!) behavior

  • All files present in the /posts directory receive the "post" tag from posts.json in addition to any that they have defined in the frontmatter.
@chrisdmacrae

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2018

Curious about discussion on this for any array defined in the stack.

So the same array defined in layout > data file > front matter all get merged into a single array.

Or should this only apply to certain "built-in" constructs?


I'm picturing non-arrays would get replaced. Not sure where we should stand on objects, though. Deep merging gets hairy.

@zachleat zachleat added the bug label Jun 26, 2018

@zachleat

This comment has been minimized.

Copy link
Member

commented Jun 26, 2018

I agree that this is a bug because it’s bitten me too 😅 and it took me a bit to figure out where I’d gone wrong.

Probably as simple as switching to use lodash.merge instead of Object.assign there.

However, I limit my agreement specifically to the tags array.

It could be just as unintuitive to switch this for other non-scalar types (arrays or objects). So… would it be gross to only apply this change to tags and nothing else? Not sure.

@philhawksworth

This comment has been minimized.

Copy link
Author

commented Jun 26, 2018

Yeah I can see where the logic (and communicating the logic) could get a little wobbly.

The only usecase I've stumbled upon for wanting these merged rather than replaced has been for tags so personally, I'd be very happy with that behavior for tags.

I am a little nervous about the introduction of some "special case magic" just for one type of property in front matter... just in the interest of clarity and comprehension. Guessing that the docs will need to work hard to make that behavior really clear.

@kleinfreund

This comment has been minimized.

Copy link
Contributor

commented Jun 27, 2018

Somewhat unelegant but avoiding the problem of special-casing only one thing: Making it explicit by merging when tags are set via a extra_tags field and overriding when they are set via a tags field.

But then again, this seems really unelegant.

@zachleat

This comment has been minimized.

Copy link
Member

commented Jun 27, 2018

We could introduce some kind of key naming convention that either opts into merging (or not)

Uh, just super quick example that I am not tied to at all:

---
__tags:
  - this-will-merge
---

or we could make the default merge and opt-in to not merge:

---
__tags:
  - this-will-overwrite
---

Just an idea

@philhawksworth

This comment has been minimized.

Copy link
Author

commented Jun 27, 2018

I'm not sure if I can quite parse the examples above @zachleat.

Could enabling the behavior be made really explicit in the folder's .json file? Something like...

posts.json:

{
  "tags" : ["post", "tag-1", "tag-2"],
  "layout" : "my-layouts/posts.njk",
   ...
   // optional options object to specify any properties 
   // which the files should extend rather than override,
   "options" : {
     "extend" : [
         "tags",
         "anything-other-property-to-extend-rather-than-override"
      ]
   }
}

The potential downsides here:

  • it makes options a reserved word that could not be used in the frontmatter
  • it adds a bit of complexity to those gloriously simple .json files
@zachleat

This comment has been minimized.

Copy link
Member

commented Jun 27, 2018

Sorry, I didn’t explain that well. I mean to add a prefix to data keys that will opt-in to the merge behavior.

For example, given the current behavior of Object.assign overwrites:

---
merge:tags:
  - this-will-be-merged
  - and-this
---

Anything with that merge: prefix on the key (at least at the top level of the data) will be merged with tags rather than overwriting tags.

@philhawksworth

This comment has been minimized.

Copy link
Author

commented Jun 27, 2018

oooh. Yummy.

@kleinfreund

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2018

One thing: Converting an existing site to use Elventy that already has posts with a tags field in the front matter. There, it would be a bit annoying to convert all tags fields to merge:tags or whatever. I think the tags case is one where merging by default is quite expected.

(Yeah, this has bitten me just now. The Eleventy debug output revealed the error.)

@kleinfreund

This comment has been minimized.

Copy link
Contributor

commented Sep 20, 2018

I think data should always merge while the sources further down the following list take precedence.

  1. project data (a.k.a. global data)
  2. directory data (e.g. posts/posts.json)
  3. file data (e.g. posts/2018-09-20-thing.md)
@zachleat

This comment has been minimized.

Copy link
Member

commented Sep 20, 2018

I believe, hopefully more accurately, that the ideal default behavior (from my perspective) is to merge Objects and Arrays will overwrite.

@edwardhorsford

This comment has been minimized.

Copy link

commented Sep 20, 2018

Heh, I just raised this exact thing in (#247).

FWIW I'd like it for more than just tags. I'd like to be able to use it for classes to be set in templates too.


Some thoughts:
I'd have a preference to the smarts being in the data file, not in the individual post. This helps for @kleinfreund's issue of existing sites with tags, but also means that authors of posts don't need to know about the magic of the blog.

I take the view that .json files are more 'authoritative' - and that with things like Netlify CMS while we may let people edit posts, we might not let them edit the data files. With the smarts being in the individual post, an author could opt out of the .json. Or more likely, they could accidentally break things.

Some ideas for .json data files:

{
  "permalink" : "/{{page.fileSlug}}/",
  "+tags" : "post"
}

Which also might let you do something like
"-tags" : "admin" // blacklist of tags you can't apply

Or more explicitly:

default : {
  "permalink" : "/{{page.fileSlug}}/",
  "category" : "design"
},
append : {
 "tags" : "post"
}
@paulrobertlloyd

This comment has been minimized.

Copy link
Contributor

commented Sep 25, 2018

👍 to this general discussion. I too thought tags would work as @philhawksworth suggested and had a moment of zen about how 11ty works 🤯 only for it to be crushed given the current behaviour 😭

As to a preference, I would rather add configuration directives regarding merging/replacing in the data file, rather than in individual content files.

zachleat added a commit that referenced this issue Oct 4, 2018

@zachleat

This comment has been minimized.

Copy link
Member

commented Oct 4, 2018

A very rudimentary working prototype of this is now available in the data-deep-merge branch. It’s enabled by default for now for all data, I haven’t quite decided how to expose this yet.

@zachleat zachleat added the in-progress label Oct 4, 2018

zachleat added a commit to 11ty/11ty.io that referenced this issue Oct 7, 2018

@zachleat

This comment has been minimized.

Copy link
Member

commented Oct 7, 2018

I’ve added the notion of Experiments to allow y’all to try this out globally and we can decide how to move forward after a wider trial.

Read more: https://www.11ty.io/docs/config/#data-deep-merge

@zachleat zachleat removed the in-progress label Oct 7, 2018

@zachleat zachleat added this to the Next Patch Version milestone Oct 7, 2018

@zachleat

This comment has been minimized.

Copy link
Member

commented Oct 7, 2018

This will be included with the next version (as of right now, that’ll be 0.6.0).

@zachleat zachleat closed this Oct 7, 2018

@kleinfreund

This comment has been minimized.

Copy link
Contributor

commented Oct 7, 2018

FYI, docs read "addExeriment".

@zachleat

This comment has been minimized.

Copy link
Member

commented Oct 8, 2018

Added a note about the override: prefix to the docs: https://www.11ty.io/docs/config/#data-deep-merge

@zachleat

This comment has been minimized.

Copy link
Member

commented Nov 17, 2018

Had a bead to think about this and I’m going to move away from the “experiments” naming structure here and just make this a top level config option. Gonna re-open until that work is done.

@zachleat zachleat reopened this Nov 17, 2018

@zachleat zachleat self-assigned this Nov 17, 2018

@zachleat zachleat closed this in bc707ad Nov 20, 2018

zachleat added a commit that referenced this issue Nov 20, 2018

@zachleat

This comment has been minimized.

Copy link
Member

commented Nov 20, 2018

@kleinfreund

This comment has been minimized.

Copy link
Contributor

commented Nov 20, 2018

Oh, I love this. 🚀

@idiazroncero

This comment has been minimized.

Copy link

commented Nov 20, 2018

Well, this is awesome

@paulrobertlloyd

This comment has been minimized.

Copy link
Contributor

commented Nov 21, 2018

Fantastic! Ship it already! 😉

@philhawksworth

This comment has been minimized.

Copy link
Author

commented Nov 21, 2018

@paulrobertlloyd

This comment has been minimized.

Copy link
Contributor

commented Nov 21, 2018

Fantastic! Ship it already! 😉

Okay, I might need to row back my eagerness a little! I’ve updated to my site to point the latest code on master, and it would seem that enabling eleventy.setDataDeepMerge(true); breaks pagination. The correct number of paginated pages are generated, but each contains the full list of items from a collection (and increasing overall build time as you might expect).

I also get the following error relating to my pagination include, although only for this one page (it’s included on many others), and removing it has no effect on the above issue:

Error writing templates: (full stack in DEBUG output)
> Having trouble writing template: www/articles/sources/index.html (TemplateWriterWriteError)
> Having trouble rendering liquid template ./src/_content/indices/articles-by-source.liquid (TemplateContentRenderError)
> Cannot read property 'slice' of undefined, file:/Users/paulrobertlloyd/Sites/paulrobertlloyd-v4/src/_includes/pagination.liquid, line:9 (RenderError)
> Cannot read property 'slice' of undefined (TypeError)

Here’s the template:

{%- if pagination.pageLinks.size > 1 -%}
<nav class="pagination" aria-label="page">
  <ol>
  {%- for item in pagination.pageLinks -%}
    {%- assign url = item | permalink | replace: 'page/','?page=' -%}
    {%- if forloop.index0 != 0 -%}
      {%- assign label = forloop.index0 -%}
    {%- else -%}
      {%- assign label = label | capitalize -%}
    {%- endif -%}
    <li><a href="{{ url }}"{% if pagination.pageNumber == forloop.index0 %} aria-current="page"{% endif %} aria-label="Go to page {{ forloop.index0 }}">{{ label }}</a></li>
  {%- endfor -%}
  </ol>
</nav>
{%- endif -%}

Hope this report helps!

@zachleat zachleat reopened this Nov 21, 2018

@zachleat

This comment has been minimized.

Copy link
Member

commented Nov 22, 2018

but each contains the full list of items from a collection

@paulrobertlloyd Can you elaborate about this?

@paulrobertlloyd

This comment has been minimized.

Copy link
Contributor

commented Nov 22, 2018

@zachleat I’ll try! So, if a collection has 100 items, and my pagination is set to show 20 items per page, with eleventy.setDataDeepMerge(true); enabled, 5 pagination pages will be generated as expected, but each of these pages will display all 100 items. Does that make sense?

zachleat added a commit that referenced this issue Nov 22, 2018

@zachleat

This comment has been minimized.

Copy link
Member

commented Nov 22, 2018

@paulrobertlloyd oh, narf—my bad. I checked in the fix for this (with a test too)—can you test on your end again?

@paulrobertlloyd

This comment has been minimized.

Copy link
Contributor

commented Nov 22, 2018

@zachleat Yup, that’s fixed it! This raises another issue however (and perhaps one that should be tracked separately) regarding the sorting of paginated collections. Let me explain. Currently, I have the following collection defined in my config:

eleventy.addCollection('note', collection => {
  return collection.getFilteredByGlob('**/notes/**/*.md').reverse();
});

which I paginate like so:

---
title: Notes
pagination:
  data: collections.note
  size: 5
---
{% for item in pagination.items %}
  {{ item }}
{% endfor %}

This results in the following – and correct – pagination (10 being the most recent item, 1 the oldest):

--- Page 1 ---
10
9
8
7
6
--- Page 2 ---
5
4
3
2
1

Now, I’d been hoping that I could do away with defining so many collections in my config, and instead use tags. However, as there’s no means of declaratively reversing a collection before it gets paginated, items are now paginated like so:

--- Page 1 ---
1
2
3
4
5
--- Page 2 ---
6
7
8
9
10

Using Liquid’s reversed filter on the forloop doesn’t help here, as that results in the following:

--- Page 1 ---
5
4
3
2
1
--- Page 2 ---
10
9
8
7
6

Basically, there needs to be a means of reversing the order of items in a collection and/or the order of paginated pages. Maybe something like:

---
pagination:
  data: collections.note
  size: 5
  reversed: true
---

or perhaps:

---
pagination:
  data: collections.note.reversed
  size: 5
---

Does this make sense… or have I missed an obvious way of achieving the above without needing to use addCollection?

@zachleat

This comment has been minimized.

Copy link
Member

commented Nov 22, 2018

That seems clear @paulrobertlloyd and there is no declarative way to achieve that currently. Can you file a new feature request issue for this? Feel free to copy and paste your above comment verbatim 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.