Add PageDrop to provide Liquid templates with data

[ashmaroli]

Background

Currently Jekyll::Page#to_liquid is an unmemoized method that returns the Hash representation of a given Jekyll::Page instance. The current approach is wasteful because:

• the method is unmemoized.
• generating the Hash representation involves resolving all content methods (not all of them are memoized) even if they're not required in a template.

Proposed Solution

• Switch to using a Liquid::Drop subclass like other Core classes. (Memoizing the method might complicate it).
• To avoid changing the contract for Page subclasses, alias existing implementation as a private method which will be called if the subclass doesn't already override the :to_liquid method.
• To allow voluntary migration to using the superclass drop in subclasses, the subclass may override their :to_liquid definition to wrap the private method :liquid_drop:

  class FooPage < Jekyll::Page
    def to_liquid
      liquid_drop
    end
  end

[mattr-]

The current approach is wasteful

Waste isn't necessarily bad, particularly when the method in question works and the way it works is the least surprising way for it to work.

the method is unmemoized

Why is this bad? Why would memoizing it be better? How does a long running Jekyll instance with this data memoized affect the user experience? For the last question, I'm concerned mostly with what happens if the data underneath changes.

generating the Hash representation involves resolving all content methods (not all of them are memoized) even if they're not required in a template.

Again, on the surface, this doesn't necessarily seem like a bad idea. Why is resolving all content methods bad?

[ashmaroli]

Why is this bad? Why would memoizing it be better? Why is resolving all content methods bad?

@mattr- I think the best way to answer this is by adding a couple of tests involving a fixture site with lots of standalone pages. But, it needs to come in through the master branch so that we can compare with this branch..

I'm concerned mostly with what happens if the data underneath changes.

Valid concern and a good one at that. I'll add a Cucumber test to check if there's a problem there
and make changes if necessary.

[mattr-]

I think the best way to answer this is by adding a couple of tests involving a fixture site with lots of standalone pages.

How is this the best way? Just explain in a comment what you're actually doing with these changes and the motivation behind making them.

Valid concern and a good one at that. I'll add a Cucumber test to check if there's a problem there and make changes if necessary.

How is a test going to help here?

[ashmaroli]

How is this the best way? Just explain in a comment what you're actually doing with these changes and the motivation behind making them.

Jekyll::Document, Jekyll::StaticFile instances expose themselves to layouts via respective Liquid::Drops. This PR proposes to move Jekyll::Page into the same boat.

The primary intention (and motivation) with this is to reduce memory usage in a site with numerous standalone pages.

[mattr-]

Jekyll::Document, Jekyll::StaticFile instances expose themselves to layouts via respective Liquid::Drops. This PR proposes to move Jekyll::Page into the same boat.

The primary intention (and motivation) with this is to reduce memory usage in a site with numerous standalone pages

Perfect! This is exactly the background information I was looking for. 😄 Thanks!

+1 from me.

[DirtyF]

👍 for cohesiveness. What are the official plugins possibly impacted by this change?

[ashmaroli]

What are the official plugins possibly impacted by this change?

This is a backwards-compatible change. However, should there be an effect, it would be in
jekyll-archives, jekyll-feed, jekyll-sitemap, jekyll-redirect-from..

[ashmaroli]

@jekyllbot: merge +minor