Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify Rails initialization to only be a mountable Engine #543

Closed
bensheldon opened this issue Mar 21, 2022 · 8 comments
Closed

Simplify Rails initialization to only be a mountable Engine #543

bensheldon opened this issue Mar 21, 2022 · 8 comments

Comments

@bensheldon
Copy link
Owner

bensheldon commented Mar 21, 2022

GoodJob currently is loaded in two parts:

  • require 'good_job' which is implicitly loaded by Rails in Bundler.require
  • require 'good_job/engine which must be manually added to the Rails application.

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:

  • GoodJob currently uses Zeitwerk directly for autoloading. As an engine will loading will be managed by Rails?
  • As an engine, will the memory footprint be (significantly) larger if the Web Dashboard is not used in an application?
@jrochkind
Copy link
Contributor

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
Copy link
Owner Author

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
Copy link
Contributor

jrochkind commented Mar 21, 2022

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
Copy link
Owner Author

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
Copy link
Contributor

jrochkind commented Mar 22, 2022

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
Copy link
Owner Author

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
Copy link
Contributor

👍 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
Copy link
Owner Author

Closed by #554.

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

No branches or pull requests

3 participants