Skip to content
This repository has been archived by the owner on Mar 10, 2021. It is now read-only.

Cache store #132

Closed
wants to merge 9 commits into from
Closed

Cache store #132

wants to merge 9 commits into from

Conversation

anthonylebrun
Copy link

Implements page content caching per #122

This is a WIP pull request.

I ran into trouble getting assets in the 1.3 example to work:

18:30:01 - error: Compiling of js/app.js failed. Error: Couldn't find preset "es2015" relative to directory "/Users/plebrun/tmp/thesis-phoenix"

Looks like a known issue, but none of the suggestions worked for me: phoenixframework/phoenix#1410. Going to investigate further.

I also ran into trouble running /bin/ci:

18:31:56.056 [error] GenServer Hound.SessionServer terminating
** (RuntimeError) could not create a new session: econnrefused, check webdriver is running
    (hound) lib/hound/session_server.ex:101: Hound.SessionServer.create_session/2
    (hound) lib/hound/session_server.ex:78: Hound.SessionServer.handle_call/3
    (stdlib) gen_server.erl:636: :gen_server.try_handle_call/4
    (stdlib) gen_server.erl:665: :gen_server.handle_msg/6
    (stdlib) proc_lib.erl:247: :proc_lib.init_p_do_apply/3
Last message (from #PID<0.883.0>): {:change_session, #PID<0.883.0>, :default, []}

Looks like I might be missing some additional setup on my machine. I can dig into this too but I thought @jamonholmgren you might know what's up since you just set Hound up.

Otherwise, let me know if this direction makes sense!

I also still need to add some unit tests, which should be pretty straight forward.

Copy link
Member

@jamonholmgren jamonholmgren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really great @anthonylebrun ! I'm excited about this change. We'll want to test it pretty well. I've noted a few changes and I would like to see some tests too. Maybe we should provide a programmatic way to clear the cache too -- just thinking out loud on that. Happy to hear your thoughts.

end

def cache do
if enable_cache(), do: Thesis.CacheStore, else: store()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will change to:

def cache do
  Application.get_env(:thesis, :cache, store())
end

@@ -26,7 +26,8 @@ config :logger, :console,
config :thesis,
store: Thesis.EctoStore,
authorization: Example.ThesisAuth,
uploader: Thesis.RepoUploader
uploader: Thesis.RepoUploader,
enable_cache: true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to allow them to specify their own cache store if they want, rather than just a boolean true/false.

To do that, let's change this to cache: Thesis.CacheStore. We can still allow them to specify a cache_name if they want.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I like your solution a lot better :) The one thing I'm not crazy about though is the cache_name option - that's really a specific sub-setting. What about if we could do something like:

config :thesis, cache: {Thesis.CacheStore, [other: :options, can: "go here"]}

And then our store behavior functions could (optionally with a [] default) accept those options as a second argument like so:

def page(slug, [cache_name: name]) do
  ConCache.get_or_store(name, slug, fn() ->
    store().page(slug)
  end)
end

def page_contents(slug, opts) when is_binary(slug) do
  page_contents(page(slug, opts))
end

Users will be able to pass in configuration values from the thesis config to their own custom stores or caches without having to either hardcode it in their own modules or set up their own additional config.

Thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You've got me thinking, @anthonylebrun ... I think to be consistent, we'd use this strategy instead (note the separate Thesis.EctoStore config just below the main config):

# Configure thesis content editor
config :thesis,
  store: Thesis.EctoStore,
  cache: Thesis.CacheStore,
  authorization: Example.ThesisAuth,
  uploader: Thesis.RepoUploader

config :thesis, Thesis.EctoStore, repo: Example.Repo

config :thesis, Thesis.CacheStore,
  cache_name: "whut_whut",
  other_option: "heyo"

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! This seems more idiomatic than what I was suggesting :) I'm still learning those Elixir conventions.

# uploader: <MyApp>.<CustomUploaderModule>
# uploader: Thesis.OspryUploader
enable_cache: true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will change to:

cache: Thesis.CacheStore


def update(page_params = %{"slug" => slug}, contents_params) do
cache_delete(slug)
store().page_contents(slug) |> Enum.each(fn(%PageContent{page_id: page_id}) ->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's decouple this from Thesis.EctoStore and use a map instead of %PageContent here.

store().page_contents(slug) |> Enum.each(fn(%{page_id: page_id}) ->

We should probably review the rest of the code base and make sure we're doing that elsewhere too. :)

end)
end

def page_contents(page = %Page{id: page_id}) do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's decouple this from Thesis.EctoStore and use a map instead of %Page here.

def page_contents(page = %{id: page_id}) do


##### Page content caching

To enable content caching, add this in your config/config.exs:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe flesh out the paragraph here, give a little more background regarding why this is desirable.

@jamonholmgren
Copy link
Member

Looks like I might be missing some additional setup on my machine.

@anthonylebrun Make sure you run brew install chromedriver (if you're on Mac, or the equivalent Linux / Windows command). That might work.

We probably need to add that to the ./bin/setup script.

@jamonholmgren
Copy link
Member

@anthonylebrun I've been out of town but will take some time to work on Thesis in the coming weeks with @yulolimum . Should see some activity here soon!

@jamonholmgren
Copy link
Member

@anthonylebrun Would you want to bring this up to date?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants