prototype of jekyll hooks, encapsulated #3553

stevecrozz · 2015-03-07T23:47:57Z

This is a second attempt at #3498 (adding hooks to Jekyll). In my last attempt @envygeeks had some complaints about the lack of encapsulation. This PR is an attempt to address those concerns and try out a new idea. This is definitely not ready to merge, but I wanted to get some eyes on it before I implement and write tests for the whole thing. The hooks concept is inspired by, but in this instance not perfectly compatible with octopress-hooks, so it's a bit of a departure from #3392 by @parkr, but I think it nicely encapsulates the hook logic.

Here's a practical example of how you could use one of these hooks to do something useful: https://github.com/stevecrozz/featherblade

parkr · 2015-03-08T01:08:34Z

Don't worry about the circleci failure.

This looks pretty good! @envygeeks?

envygeeks · 2015-03-08T01:49:03Z

👍 I like it!

envygeeks · 2015-03-08T01:49:27Z

Sorry about the 👎 I actually meant to do 👍

stevecrozz · 2015-03-08T06:02:13Z

Great. I'll flesh out the rest of it and add some test coverage.

stevecrozz · 2015-03-08T21:24:46Z

This is a bit more fleshed out now, although still lacking in tests and docs. I did do some one-off tests and it all seems to be working pretty well. Every hook receives the object being "hooked into" as the first argument.

One major difference from octopress-hooks worth pointing out is how I'm dealing with the payload objects just before rendering.

In octopress-hooks, there's a special hook for merging in new payload params. But I took a different approach and just pass the payload as an argument to each of the pre_render hooks. Anyone who implements a pre_render hook will receive the payload object and can modify it there if necessary as well as do any other things that need doing before rendering. pre_render is the only hook that receives a second argument at the moment. @imathis: let me know if you have any thoughts on this, especially if you already rejected this idea for some reason.

Next up is tests and docs, but I'll give this PR a few days to bake in case there's more feedback.

stevecrozz · 2015-03-14T04:19:37Z

I'm working on cucumber tests now. How thorough do these tests need to be? Do we want each hook for every container to have its own test scenario?

envygeeks · 2015-03-14T04:29:07Z

I say just write them and if there is a problem we'll point it out!

stevecrozz · 2015-03-14T20:32:59Z

@envygeeks / @parkr I've got cucumber tests and docs written. Do you have any feedback?

parkr · 2015-03-16T21:27:34Z

site/_docs/plugins.md

@@ -469,6 +469,179 @@ module Jekyll
 end
 {% endhighlight %}

+## Hooks
+


Can you add an "unreleased" note here? Check out #3556 for an example.

parkr · 2015-03-16T21:30:12Z

Unless you have some thoughts, @envygeeks, let's merge and get this out there in a new beta so people can start building for it. Best way to fix bugs is through actual use of the platform.

stevecrozz · 2015-03-16T21:37:00Z

Amended commit to include "unreleased" note in the docs

alfredxing · 2015-03-17T21:56:59Z

👍 Nice work on this! We're missing hooks for Documents but that shouldn't be a huge issue.

imathis · 2015-03-17T22:09:51Z

I'm sorry I'm a bit late to this.

Unless I've overlooked it, this does not include:

Site: pre_read, merge_payload
Page/Post: merge_payload

For reference here's Octopress's hook timeline.

stevecrozz · 2015-03-18T01:31:21Z

@imathis That's correct. I removed pre_read from my PR while writing the test cases because I couldn't come up with a use case to test that wasn't already covered. There may be some good ones, but I didn't think of any.

As for merge_payload, I did depart from the octopress-hooks approach which is why I was interested in your feedback. I tried to cover the use case of altering the payload using pre_render which passes the payload to the callback as its second parameter. There's less code and less to test in this approach and it seems more flexible, but I'd be interested to know if I missed a good reason to implement it the way you already did.

parkr · 2015-03-30T23:57:56Z

@imathis ^^?

imathis · 2015-03-31T00:21:41Z

I'm going to create a branch of octopress-hooks that translates hooks to be based on this. If my tests pass. 👍 Let's see how long this takes :)

envygeeks · 2015-03-31T00:26:54Z

Can we focus more on what we need and less on what we have?

imathis · 2015-03-31T00:29:11Z

@envygeeks I'm focusing on what I need. I built the features in Octopress hooks because I needed them. If I can keep that functionality, I'll be good to go. Otherwise I'll have to keep monkey patching my own hooks.

envygeeks · 2015-03-31T00:33:40Z

@imathis Well if you run into trouble then please do post the hooks that are failing and where they originate and are used at so I might be able to help you two come to compromise by possibly reworking both sides to see if we can fit your hooks into his hook system or if we need to truly have him change his hook system.

stevecrozz · 2015-05-02T03:54:22Z

This should address all the feedback @imathis. Just in time for JekyllConf!

parkr · 2015-05-02T17:11:59Z

lib/jekyll/document.rb

@@ -175,11 +175,15 @@ def destination(base_directory)
    #
    # Returns nothing.
    def write(dest)
+      Jekyll::Hooks.trigger self, :post_render


Why not put this at the end of render?

That's fine with me. It's here just to match the behavior of octopress hooks:
https://github.com/octopress/hooks/blob/master/lib/octopress-hooks.rb#L256

I'm fine with moving it though. Do you still want me to do that?

Yeah, I think it'd make more sense. :)

parkr · 2015-05-02T17:20:23Z

Thanks for all your work on this, @stevecrozz!! ✨

Any other thoughts, @jekyll/core and @imathis?

envygeeks · 2015-05-02T17:21:38Z

My only complaint is that defaults aren't on the surface API, we are seeping nil through the stack into internal API's and then setting defaults. I think that's obscure design and we should surface every default we have on the layer that is public making everything obvious.

parkr · 2015-05-02T18:32:09Z

I would tend to agree, Jordon! Which defaults, specifically?

- change site:reset to site:after_reset - raise an exception when registering uncallable hook - set default hook priority at the public API level

envygeeks · 2015-05-03T05:26:00Z

@parkr it was originally on line 53 of hooks.rb but it looks @stevecrozz caught what I meant and correct it so 🎉 I'm liking this so far!

imathis · 2015-05-03T17:18:58Z

Lookin' good! I'll give another go at rebuilding some plugins based on this.

parkr · 2015-05-03T20:11:31Z

@gjtorikian: would you mind trying out this branch on a big site you have access to? Interested in performance change, if any. ❤️

stevecrozz · 2015-05-03T20:35:28Z

@imathis You could potentially author a shim to make your current octopress-hooks based plugins work with these new hooks. Here's a totally untested start at something like that.
https://gist.github.com/stevecrozz/ca294104609efd8205bb

imathis · 2015-05-04T15:23:40Z

@stevecrozz I'll give it a try, thanks for the guidance.

gjtorikian · 2015-05-04T17:10:02Z

would you mind trying out this branch on a big site you have access to? Interested in performance change, if any.

I kept trying it out and thought it would be a quick discovery, but kept running into problem. First, most plugins aren't Jekyll 3 compatible yet; I've logged a few issues for them. But even with them commented out, I discovered so of my own plugins/hacks are incompatible, too. It'll take me some time to upgrade those, in order to be able to test performance on Jekyll 3/this branch.

parkr · 2015-05-09T03:47:51Z

@stevecrozz Is this ready to go?

parkr · 2015-05-10T06:58:36Z

Merging. Feel free to add further feedback as we approach the final release!

Merge pull request 3553

[ci skip]

imathis · 2015-05-10T12:41:20Z

I just pushed an octopress-multilingual pull-request for merging Jekyll hooks support. It worked really well and was nice to integrate. Thanks for your work on this @stevecrozz and everyone else who helped! 👍

parkr · 2015-05-10T18:34:27Z

Hooray!!! 🎉

stevecrozz · 2015-05-11T03:28:54Z

Very cool. Thanks for all the help, everyone. I know there were a bunch of other ideas up there that didn't make it into this first round but it doesn't have to end here. I'll look forward to seeing and reviewing more hook-related work.

stevecrozz force-pushed the jekyll-hooks branch from b6be739 to 4ddbb63 Compare March 8, 2015 21:16

stevecrozz force-pushed the jekyll-hooks branch 3 times, most recently from 3dd9a3a to 960960e Compare March 14, 2015 04:15

stevecrozz force-pushed the jekyll-hooks branch 3 times, most recently from 8e42949 to c63551c Compare March 14, 2015 20:31

parkr reviewed Mar 16, 2015
View reviewed changes

stevecrozz force-pushed the jekyll-hooks branch from c63551c to ab3f782 Compare March 16, 2015 21:35

parkr mentioned this pull request Mar 16, 2015

3.0 RELEASE GAMEPLAN #3324

Closed

7 tasks

parkr added this to the 3.0 milestone Mar 16, 2015

stevecrozz force-pushed the jekyll-hooks branch from ab3f782 to b1660ec Compare March 24, 2015 18:52

parkr reviewed May 2, 2015
View reviewed changes

address code review feedback from jekyll#3553

736e6a7

- change site:reset to site:after_reset - raise an exception when registering uncallable hook - set default hook priority at the public API level

move document.post_render hook to after render call

79a178f

This was referenced May 4, 2015

Update dependency for Jekyll 3 jekyll/jekyll-sitemap#74

Closed

Support Jekyll 3.0 jekyll/jemoji#17

Closed

parkr added a commit that referenced this pull request May 10, 2015

Merge pull request #3553 from stevecrozz/jekyll-hooks

ce9fcfa

Merge pull request 3553

parkr merged commit ce9fcfa into jekyll:master May 10, 2015

parkr added a commit that referenced this pull request May 10, 2015

Update history to reflect merge of #3553

f34a004

[ci skip]

jekyll locked and limited conversation to collaborators Feb 27, 2017

jekyllbot added the frozen-due-to-age label Feb 27, 2017

prototype of jekyll hooks, encapsulated #3553

prototype of jekyll hooks, encapsulated #3553

Conversation

stevecrozz commented Mar 7, 2015

parkr commented Mar 8, 2015

envygeeks commented Mar 8, 2015

envygeeks commented Mar 8, 2015

stevecrozz commented Mar 8, 2015

stevecrozz commented Mar 8, 2015

stevecrozz commented Mar 14, 2015

envygeeks commented Mar 14, 2015

stevecrozz commented Mar 14, 2015

parkr Mar 16, 2015

Choose a reason for hiding this comment

parkr commented Mar 16, 2015

stevecrozz commented Mar 16, 2015

alfredxing commented Mar 17, 2015

imathis commented Mar 17, 2015

stevecrozz commented Mar 18, 2015

parkr commented Mar 30, 2015

imathis commented Mar 31, 2015

envygeeks commented Mar 31, 2015

imathis commented Mar 31, 2015

envygeeks commented Mar 31, 2015

stevecrozz commented May 2, 2015

parkr May 2, 2015

Choose a reason for hiding this comment

stevecrozz May 2, 2015

Choose a reason for hiding this comment

parkr May 3, 2015

Choose a reason for hiding this comment

parkr commented May 2, 2015

envygeeks commented May 2, 2015

parkr commented May 2, 2015

envygeeks commented May 3, 2015

imathis commented May 3, 2015

parkr commented May 3, 2015

stevecrozz commented May 3, 2015

imathis commented May 4, 2015

gjtorikian commented May 4, 2015

parkr commented May 9, 2015

parkr commented May 10, 2015

imathis commented May 10, 2015

parkr commented May 10, 2015

stevecrozz commented May 11, 2015