bensheldon / good_job

Multithreaded, Postgres-based, Active Job backend for Ruby on Rails.
https://goodjob-demo.herokuapp.com/
MIT License
2.69k stars 200 forks source link

Simplify Rails initialization to only be a mountable Engine #543

Closed bensheldon closed 2 years ago

bensheldon commented 2 years ago

GoodJob currently is loaded in two parts:

This happened because the Engine came 2nd and part of some initial reluctance on my part around the Web Dashboard, as well as my concerns about memory/code footprint of an Engine if the application was not explicitly using it. At this point though...

I think GoodJob should simply be an engine, and if one isn't using the Web Dashboard they simply would not mount the routes.

Some open questions:

jrochkind commented 2 years ago

Makes sense to me.

If something works only with Rails, I don't think there is any purpose to allow someone the option of loading the Engine class or not.

The pattern I see most engines do -- and which I think plugin new --full does -- is that ./lib/gem_name.rb itself will include a require 'gem_name/engine'. So the engine class gets automatically required, there is no need for the consuming app to manually do a require 'gem_name/engine', and also no way to avoid loading the Engine class. the Engine class is just automatically loaded on gem require. this is an implementation detail that most users don't need to be aware of -- most people who don't write rails plugins probably arne't even aware there is such a thing as an "engine class", and they really don't need to be. It's just an implementation detail of how Rails integration works.

(I think there is a lot of "legacy" stuff that doesn't always make complete sense in the Rails plugin/engine stuff, especially the docs, because they don't get a lot of attention from Rails maintainers. So there are some things, like the distinction between "plugin" and "engine" -- that don't really make a lot of sense, but are just kind of leftovers from the history, and the docs could probably use a large refactor too, but probably won't get it).

bensheldon commented 2 years ago

My intention of splitting them was entirely about memory footprint. I didn't actually profile it (😬) but the intent was that if you were not using the Web Dashboard, your application would not incur the memory footprint cost of the Controllers/View Helpers/Cached ERB templates (not sure if these get preloaded/cached in production mode). I don't imagine it would be more than a few megabytes (again, I should actually profile it), but streamlining things for Heroku is always on my mind.

jrochkind commented 2 years ago

Yeah, some things will probably get preloaded in production mode. Certainly any .rb files in ./app will. Not totally sure about the .html.erb templates, although I would predict them to be barely measureable RAM.

As a heroku user myself, I doubt I'd think any RAM savings here (probably not too large) are worth any added complexity in maintenance or use, but okay, I see!

If you ever need any Rails Engine features for something not related to the dashboard -- which seems plausible to me -- the approach of letting "load the Rails engine class" be a proxy for "load the dashboard" would start failing! In general, Rails is not so much about letting you selectively load features like this, but if you really wanted to, I wonder if there'd be another way, instead of avoiding loading the engine class, using Rails features to take your stuff out of the autoload paths or something.

bensheldon commented 2 years ago

I appreciate your perspective on this. You're building up my sense that I should just turn it entirely into an Engine and move forward.

jrochkind commented 2 years ago

Yeah, just my thoughts, it's probably fine either way!

The real straightforward/natural way to make the dashboard be optional, and not loaded if not opted into... is to make it a separate gem! that an adopter can add to their Gemfile or not. That of course has it's own maintenance burden.

bensheldon commented 2 years ago

Totally. I've been opinionated that if I'm maintaining it, it's going into GoodJob directly.

I'd love for someone to lead on dogfooding a proper hook/plugin system for GoodJob though.

bkeepers commented 2 years ago

👍 I like the idea of simplifying it. I can't imagine the memory overhead would be anything significant in its current state.

Another option is to publish a separate gem (like good_job-ui…sorry to mix _ and -) from the same repository, which would load the engine. This is how we handle it with flipper.

In this case though, I think most people will (or should) want both, so I'm not sure the separation is necessary.

bensheldon commented 2 years ago

Closed by #554.