samvera / questioning_authority

Question your authorities
Other
54 stars 30 forks source link

Support for Rails 7.0.x #377

Closed jrochkind closed 1 year ago

jrochkind commented 1 year ago

We've been struggling with Rails autoloading for a while, around reference to Qa::LinkedData::AuthorityService in ./config/initializers/linked_data_authorities.rb

In Rails 6.1, it produced a deprecation warning as in #349, a PR that tried to resolve the deprecation warning. However that PR caused problems for downstream apps, and was reverted in #350.

Trying to test the app in Rails 7 -- it's moved on from deprecation warning to simply not working, with an error:

questioning_authority/config/initializers/linked_data_authorities.rb:1:in `<top (required)>': uninitialized constant Qa::LinkedData (NameError)
    from /Users/jrochkind/.gem/ruby/3.0.4/gems/railties-7.0.4/lib/rails/engine.rb:667:in `load'

I understand that in modern zeitwerk, you can't reference files that will be development load reloaded in an initializer (see deprecation warning in #349). But I'm a bit surprised and don't totally understand why it applies here, since files in an engine I thought weren't reloaded anyway.

Nonetheless, it looks like it works to move the file defining Qa::LinkedData::AuthorityService from ./app to ./lib, and add an explicit require of it at boot, in the engine.rb file. Tests now pass. I tried that because I knew ./lib wasn't involved in any kind of Rails autoloading, but can't say I 100% understand what's going on.

Con this change:

Pro this change:

@cjcolvar since the last time I tried a fix you are the one who noticed it breaking an app... do you have any interest in testing this branch with whatever the last try broke, to ensure it's good this time before we merge?

Would close #351 I think

jrochkind commented 1 year ago

Note this PR includes adding 7.0.x to test matrix, and it passes!

jrochkind commented 1 year ago

Thanks @cjcolvar!

I'm going to do a tag and release too, unless someone tells me not to.