Ensure .pagination.next is not present for oldest year

[XhmikosR]

@phillipj: this seems to trigger the failure which is the issue we are hitting upstream. Not sure if I'm missing something, though.

Fixes #5

[phillipj]

Really nice! This is very valuable when trying to grasp what on earth is causing this bug in the first place 💯

I'll do my best to squeeze in a thorough look tomorrow and see if I remember much of how this works.

[XhmikosR]

Friendly ping @phillipj :)

Maybe we should explicitly set this to null or undefined when we are at the last year?

[phillipj]

Yo, thanks for the ping!

I checked out this branch a couple of days ago and tried to grasp what this code actually does.. I'll be honest and say the cloning involved in ./index.js#L42 and below wasn't really obvious at first glance.

Your suggestion of always setting .next to undefined|null (or not set it at all?) when we're looping through posts for the last year, sounds sensible. Haven't tried it myself yet, but guess that's what would be most sensible, especially when seeing what you discovered in the nodejs.org repo.

[XhmikosR]

I'm also getting lost in that clone part, so I couldn't make a lot of sense of it.

metalsmith-yearly-pagination/index.js

Lines 48 to 55 in 3a305bc

	last.pagination.next = clone;
	clone.pagination.year = year;
	clone.pagination.prev = last;
	clone.pagination.posts = posts.map(iteratee);
	files[cloneName] = clone;
	last = clone;

Basically when we are at years[years.length - 1] I think we should not set it or null it.

[phillipj]

👍

If not setting .next make the tests added happy, you could probably verify if that makes the nodejs.org issues go away with npm linking locally?

I'm more than happy to push a new version this weekend if needed.

[XhmikosR]

I don't have a working patch. I was hoping you could help me out here because I don't quite get all that cloning.

Before releasing anything, we'll verify it of course :)

[phillipj]

Alrighty, I'll give it a try locally and see it affects the added tests 🤞

[phillipj]

Will try more later, first naive attempt didn't yield expected results I'm afraid.

[phillipj]

Found a way to make all the tests pas.. You okey with me pushing a commit to your branch?

[phillipj]

Pushed my trivial fix (incl/your tests) to fix-next-pagination-in-oldest-year in case you want to do some early stage verification.

[XhmikosR]

You could push here ofc :)

Anyway, I tested the branch and seems to be fine upstream. Could we miss something else?

[XhmikosR]

Only thing that bugs me is that we shouldn't set the property in the first place. Not sure how much it would complicate things, but it feels the right thing instead of deleting the property :)

[phillipj]

Myeah, totally see your point there. Although deleting the .next property works, I agree it's not ideal.

When trying out other approaches, I ended up feeling like there's some mutable references that really caught me off guard and therefore ended up causing more headache than I cared to understand.

Which obviously raises an obvious need to refactor the inner workings a bit, so that our future selfs understands what the heck is going on.

I'm willing to try a last shot at understand & refactoring the cloning and reference parts, but I got other things on my table today as well, so I won't spend too much time on it. In worst case, we've now got a plausible solution by deleting the .pagination.next.

[XhmikosR]

Agreed. I'll try experimenting with this and let you know. But perhaps you should merge this and your patch? Just don't release it yet

…

On Sat, Sep 28, 2019, 12:06 Phillip Johnsen ***@***.***> wrote: Myeah, totally see your point there. Although deleting the .next property *works*, I agree it's not ideal. When trying out other approaches, I ended up feeling like there's some mutable references that really caught me off guard and therefore ended up causing more headache than I cared to understand. Which obviously raises an obvious need to refactor the inner workings a bit, so that our future selfs understands what the heck is going on. I'm willing to try a last shot at understand & refactoring the cloning and reference parts, but I got other things on my table today as well, so I won't spend too much time on it. In worst case, we've now got a plausible solution by deleting the .pagination.next. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#9?email_source=notifications&email_token=AACVLNN2FR4YKF454VUFJITQL4NAZA5CNFSM4IX56VKKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD72UP4I#issuecomment-536168433>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACVLNP56XF4XXHL55SDGGDQL4NAZANCNFSM4IX56VKA> .

[phillipj]

Ohohoho I finally get it!

Personally see this as a bug from multiple levels of mutable references, where we set .pagination.next in one iteration of years.forEach() and we get that value with us in the next iteration as well. My ramblings probably doesn't make sense, but I'll get a fix in place to ensure this mindbending experience doesn't hit us as hard again later.

Also sadly have realised there's no way around having to mutate references being provided by metalsmith, as it seems that's really the "API" metalsmith plugins use; alter the posts & collections in place. My brain would really appreciate if we could avoid mutating references like that, but that seems impossible with how metalsmith works as far as I see things.

[phillipj]

Previously, we mutated the .pagination object which was a copy of last year's pagination object -- that included the .pagination.next property.

Now that we're always re-creating a new .pagination object, we're ensuring that the last year's .pagination.next isn't present anymore.

That property is only set in the next iteration, which doesn't happen for the oldest year -- voila, bug fixed.

[phillipj]

Rebased with master and included a better fix than the delete .. fix discussed earlier.

Also included a couple of variable renamings which surely helped my brain grok what was going on.

Shout if you don't really agree this was much better than deleteing, or else I'll merge within a day or two.

[XhmikosR]

LGTM as far as I can tell. I tested it with nodejs.org and works as expected. Good job!

[XhmikosR]

Scratch my comment about this being slower; we run more tests :)

[phillipj]

Cool! Thx a lot for your patience and pushing for getting this bug fixed in the right place, rather than in using projects 👍

[XhmikosR]

Let me know when you release a new version so that update it upstream :)

[phillipj]

v4.0.0 is available on npm now 🚀