stimulusreflex / stimulus_reflex

Build reactive applications with the Rails tooling you already know and love.
https://docs.stimulusreflex.com
MIT License
2.28k stars 172 forks source link

Development caching is confusing #258

Closed leastbad closed 4 years ago

leastbad commented 4 years ago

There was a discussion on the Chatter repo about how things would act strangely if caching is not enabled in development. I believe that I have fully cracked the code but I feel the scenario should be revisited.

Long story short: we don't (currently?) support cookie-based sessions. Instead, in the StimulusReflex install rake task we actually set the session storage to be cache_store and then enable caching in development as part of the task.

There are two very real issues with this:

First, some people might have legitimate reasons to not want caching enabled in development. Honestly, I don't understand the logic of developing with caching off, considering that if you're going to have bugs in production, development seems like a pretty good time to catch them. I personally believe that development caching should be on by default for this reason - but I don't get to make this call. If they need to have caching off and use StimulusReflex, they can use ActiveRecord session storage.

Second, the bigger problem is that a lot of problems emerge when people clone repos that use StimulusReflex and they don't know to explicitly turn caching on. It's done for you when you install SR, but you're on your own when you're cloning. A significant and growing percentage of support concerns on Discord are resolved by getting developers to enable caching.

I have several possible solutions to suggest:

  1. We could expand on our current opinionated defaults and instead of just adding config.session_store :cache_store to config/environments/development.rb on line 3 as we do now, we could actually remove lines 19 and 27-31 from development.rb, essentially removing the rails cache:dev locked in the ON position.
  2. We could create tmp/caching-dev.txt if it doesn't already exist as part of the rake task.
  3. We could install a git hook to create tmp/caching-dev.txt if it doesn't exist at checkout.
  4. We could do:
    config.cache_store = :memory_store
    config.action_controller.perform_caching = false

None of these are perfect, but what's clear to me is that every outcome has compromises attached to it, including the status quo.

julianrubisch commented 4 years ago

I‘d opt for the second along with an info log message.

Also, I remember having noted that we should probably add that config line to production, too. Tripped me up a couple of times

andrewmcodes commented 4 years ago

I concur with @julianrubisch that #2 seems like the best way to go as long as we make sure to alert the developer properly and not with a message that flashes by in the logs detailing what we did and why. Great writeup @leastbad

leastbad commented 4 years ago

If anyone wants to make number 2 into a PR, that'd be pretty rad.

RolandStuder commented 4 years ago

@leastbad I could have a go at that. Any cross OS thing I should look out for?

leastbad commented 4 years ago

@RolandStuder amazing! I think Rails.root.join("tmp/caching-dev.txt") should git'r done.

RolandStuder commented 4 years ago

I tried to have a go at this, but I have very little of the bundling / gem install process. If I understand correctly the Rakefile says how to build / install a Gem. SR does this using bundler helpers by require "bundler/gem_tasks" So I feel like the way to go about this is to replace / extend the required install task. But I don't see any effect of changing anything in the Rakefile.

I am including the gem via the path.

Can somebody give me any pointers, how to go about this?

leastbad commented 4 years ago

I'm not 100% sure I understand what you're asking, but I'm also pretty new to gem stuff.

The install task can be found in the project at lib/tasks/stimulus_reflex/install.rake

Not sure where (or if) Rakefile comes into the conversation.

RolandStuder commented 4 years ago

Well the point is, with the install task it already does activate the cache, see

https://github.com/hopsoft/stimulus_reflex/blob/master/lib/tasks/stimulus_reflex/install.rake#L54

I thought the point was to make a line that does basically the same thing (make sure there is a tmp/caching-dev.txt), but at the moment when you just have a Gemfile and run bundle install.

Well it may be that this is not actually possible.

Or maybe I misunderstood the intention of solution 2 alltogether :-)

leastbad commented 4 years ago

Sorry, you're absolutely right. I was initially trying to answer your question without a full context switch. That didn't work out so well, I'm sorry.

I don't know the solution, so anything that follows is just brainstorming with you. I don't know what's considered to be "too opinionated" when it comes to Rails autoload stuffs. What I'm about to say could be too heavy-handed.

First: I think where we should be looking (instead of Rakefile) is lib/stimulus_reflex.rb aka the place where everything gets required.

Second: I think it's too late for this particular execution to turn on caching if it's not already enabled when your environment config file runs. The most we can do is inspect the values and then, depending on how they are set, consider putsing a strongly worded message in the server output logs.

[6] pry(main)> Rails.application.config.session_store
=> ActionDispatch::Session::CacheStore
[7] pry(main)> Rails.application.config.cache_store
=> [:redis_cache_store, {:url=>"redis://localhost:6379/1"}]

Now, if we wanted to be radical, we could:

I don't think we should do that, currently. But I do like the idea of inspecting the current configuration details and then potentially displaying a message. What do you folks think?

excid3 commented 4 years ago

I submitted a PR for this just now which uses an initializer in the Engine to verify caching is enabled and exit out if it isn't. 👍

I've had quite a few people trying to use Stimulus Reflex in Jumpstart Pro and they think Jumpstart is broken when it's actually Stimulus Reflex's caching requirement they weren't aware about. This should help a lot.