Configure load paths of controllers and models

[afrase]

Hey thanks for the work on this gem. I saw the note on wrapping the initializer in Rails.application.config.to_prepare and thought I would fix that. I'm not sure how open you guys are to PRs.

• Autoload once the app/controllers and app/models directories to prevent the uninitialized constants error.
• Removed require_dependency since it's obsolete and not needed with autoloading.
• Removed the note in the readme about Rails 7 NameError.

[RoxasShadow]

  1) Zeitwerk compliance eager loads all files without errors
     Failure/Error: expect { Rails.application.eager_load! }.not_to raise_error

       expected no Exception, got #<NameError: uninitialized constant Scimitar::ActiveRecordBackedResourcesController::ActiveRecord> with backtrace:

seems like with Mongoid I am still getting the same issue

[afrase]

@RoxasShadow Do you have more details on your setup? The error implies that ActiveRecord doesn't exist since the missing constant is Scimitar::ActiveRecordBackedResourcesController::ActiveRecord.

[RoxasShadow]

@RoxasShadow Do you have more details on your setup? The error implies that ActiveRecord doesn't exist since the missing constant is Scimitar::ActiveRecordBackedResourcesController::ActiveRecord.

Yes indeed, as I use Mongoid, and only load ResourcesController but Zeitwerk includes the AR resource when eager loads.

[afrase]

@RoxasShadow I was able to replicate this.
So one fix could be to use rescue_from with a string to prevent it trying to reference the uninitialized constant. rescue_from already converts the classes into strings anyway.
Another would be to skip eager loading app/controllers which rails engines are automatically setup to do.

I pushed a commit to use a string for rescue_form since it seemed like the simplest fix.

[RoxasShadow]

That fixed the issue! Thanks a lot

[pond]

There's more to it than that; in development, it'll mean Scimitar initialisation is checked if altered, on reload. Otherwise it's not.

https://guides.rubyonrails.org/configuring.html#initialization-events

Both the Rails 6 and Rails 7 versions of the gem support (and like many other gems) require to_prepare for standard initialisation if using ActiveRecord, with the Rails 6 gem having a fallback mode for people who weren't already doing that but were getting away with it (Zeitwerk can be weird).

As a result I don't really see what this PR is for, especially since it introduces inherent and hard-to-test fragility for anything that might be subject to autoloading in future and would need the to_prepare block.

@RoxasShadow I don't understand. If you're not using ActiveRecord, you shouldn't be using the AR-backed resource controller. If you're saying that with no use of it, Scimitar's default inclusion chain as a gem means it gets read anyway and as a result you get an undefined constant error, then that's a separate bug / issue and should be addressed in another PR (specifically, not including the AR-backed resources controller; it's IMHO not the right thing to just try to work around around the reference via a string in the rescue_from - that's just trying to patch the symptom, but isn't fixing the problem).

[afrase]

@pond You are right about reloading constants from extended schemas in development which I hadn't thought about. This change doesn't stop you from using the to_prepare block though.

So this PR just instructs Zeitwerk to autoload the controller and models directory and not eager load them. Autoloading basically just primes the constant lookup table before user defined initializers are ran. Autoloading actually doesn't read the contents of the files interestingly. So I can't really think of how this could add fragility since non-existent folders/files are just ignored. Maybe if you didn't follow zeitwerks naming rules? 🤔 You might have other issues though.

Actually thinking about @RoxasShadow issue, this gem as it is won't work without ActiveRecord when eager loading is enabled in production mode anyway with the same issue.

Feel free to close this if you like.

[pond]

@afrase I like your cleaning up via removing the requirements lines and adding the load paths into the engine. Reads more like a bug fix than anything else, in fact.

I see no reason to remove the README.md docs on to_prepare, but perhaps lessen the wording (looks it needs updating anyway since Rails 6 has that mechanism too now). The issue related to needing a String version of that ActiveRecord check is a curious one; I think it's all getting pulled in automatically as part of being an Engine, since there's no explicit require for that ("ordinary" gems of course must require their various components directly and I'd intended to put a if defined?... check against it for the ActiveRecord-based parts, but that's not what's happening here).

So - much as using the string in rescue is kinda horrid, I can't (given the above) see a cleaner solution. I think my plan is:

• Merge this
• Reinstate some of the README stuff
• Keep the ::Rails::Engine as ISTR that was necessary somewhere back in the depths of time; any reason why you removed the :: prefix to force root namespace lookup?

[pond]

@afrase So, I merged this into a branch and arising is:

#112

...for a final merge of the combined work into main; does that look OK?