Sort JSON-LD data by key

[parkr]

Deterministic output is desired to improve diffability between builds.
If the output randomly changes order each time it's built, then every
page on a given site will change with every Jekyll build. Tools like
rsync and git which can diff files will always show a change even when
the content hasn't truly changed.

Fixes #362. Fixes #436.

[ashmaroli]

I recommend optimizing as much as possible to reduce impact on performance.

[parkr]

@ashmaroli I figured we could make this a first-class thing and just change to_json to return a sorted object instead of adding a second method.

[parkr]

Ok I decided to measure the .sort_by(&:first).to_h I'm adding. It's not great:

Warming up --------------------------------------
            original    55.477k i/100ms
        with sort_by    30.221k i/100ms
Calculating -------------------------------------
            original    539.839k (± 8.8%) i/s -      2.718M in   5.078878s
        with sort_by    322.651k (± 2.8%) i/s -      1.632M in   5.062437s

I went up the chain and am taking advantage of the fact that Hashes preserve insertion order. So if you insert "@context" before "@type", then iterating over the Hash (and serializing it to JSON) will give you "@context" first, and "@type" second consistently. I overwrote the original Drop#to_h to sort the keys.

[ashmaroli]

taking advantage of the fact that Hashes preserve insertion order.. overwrote the original Drop#to_h to sort the keys.

This is a wonderful idea! Good job! 👏

I think this is sufficient for an approval now, but definitely could be optimized further..

Essentially, the json_ld.to_json call currently consists of numerous iterations:

def to_json(state)
  keys.sort.each_with_object({}) do |(key, _), result|
    result[key] = self[key]
  end.compact.to_json(state)
end

👉 .sort.each_with_object.compact

I'll think about this till tomorrow. and shall register my approval then.
Will merge after Matt provides his inputs too.

Thank you very much @parkr

[parkr]

@ashmaroli Great point! Further optimized to add an if <value> to the each_with_object. I think we need to keep keys.sort but a good optimization is to get rid of compact if not necessary!

[ashmaroli]

👍

However, this PR may have exposed a bug in Jekyll Core.. Specifically:

keys.sort.each_with_object({}) do |(key, _), result|

From Jekyll Core codebase Drop#keys is a flat array:

      def keys
        (content_methods |
          mutations.keys |
          fallback_data.keys).flatten
      end

Why is the .each_with_object block destructuring the array into pairs?

keys.sort.each_with_object({}) do |(key, _), result|

instead of:

keys.sort.each_with_object({}) do |key, result|

---

UPDATE: Apparently, it's just an implementation bias.
array.each_with_object({}) do |(key, _), result| is functionally equivalent to array.each_with_object({}) do |key, result|

[parkr]

Yeah I'm not sure why we did that. I'm going to leave it as-is to match Jekyll Core.

[ashmaroli]

One final edit and this is good to go.

[ashmaroli]

pinging @mattr- for 👀

[parkr]

@ashmaroli let's give Matt until the weekend. If he's not free to take a look by then, then I think your and my approval counts as 2 Jekyll maintainers' approval so we can merge and cut a release.

[ashmaroli]

Thanks @parkr
@jekyllbot: merge +minor

[ashmaroli]

@jekyllbot: merge +minor

[parkr]

Thanks @mattr-! Thanks @ashmaroli!