Comply with Zeitwerk autoloader #3465
Conversation
This is a breaking change, it requires all gems that currently depend on
something in Refinery by requiring the gem's name to update the require
to a path inside the gem.
For example, if a gem had:
require 'refinerycms-pages'
This has to change to:
require 'refinery/pages'
Not a huge change, but needs to be done everywhere.
parndt
changed the title
| @@ -1,7 +1,7 @@ | |||
| require 'thor' | |||
|
|
|||
| module Refinery | |||
| class CLI < Thor | |||
There was a problem hiding this comment.
perhaps it should live inside Core too?
There was a problem hiding this comment.
I think we could leave it here until we rewrite it
in order to comply with Zeitwerk loader conventions
bricesanchez
dismissed
their stale review
External gems like refinerycms-wymeditor does not load anymore
because gem name and lib are not the same since we comply to Zeitwerk convention
Gemfile
Outdated
| @@ -15,10 +15,10 @@ gem 'listen' | |||
| gem 'activejob' | |||
|
|
|||
| # Add support for refinerycms-acts-as-indexed | |||
| gem 'refinerycms-acts-as-indexed', ['~> 3.0', '>= 3.0.0'] | |||
| gem 'refinerycms-acts-as-indexed', ['~> 3.0', '>= 3.0.0'], require: 'refinery/acts_as_indexed' | |||
There was a problem hiding this comment.
still wishing there is a way we can get this without the require. In this case I guess the gem should be called refinerycms-acts_as_indexed
There was a problem hiding this comment.
It could help but there will always be a problem on the cms part
There was a problem hiding this comment.
not if it's lib/refinerycms/acts_as_indexed.rb
Maybe for now we can use lib/refinerycms/acts/as/indexed.rb? Haven't tried it yet
|
@bricesanchez not exactly a monkey patch, we're just defining our own inflector right? |
parndt
changed the title
parndt
dismissed
bricesanchez’s stale review
let's revisit as approach has changed
|
This seems to be almost done. It would be nice to see it merged. |
|
Does it work for you @RandieM ? |
Sorry for the late reply. The error is gone and the website seems to work without issues. Let me know if you'd like me to test something more specific. |
|
@RandieM thanks 😄 |
This adds Zeitwerk support like refinery/refinerycms#3465 Co-authored-by: Brice Sanchez <bricesanchez@users.noreply.github.com>
| require 'refinerycms-core' | ||
| require 'refinerycms-dragonfly' | ||
| require 'refinerycms/core' | ||
| require 'refinerycms/dragonfly' |
There was a problem hiding this comment.
Shouldn't this be 'refinery/dragonfly' then? I still get " cannot load such file -- refinerycms/dragonfly (LoadError)" as I try to upgrade to rails 6
Context
For full Zeitwerk support, we require all gems that currently depend on
something in Refinery by requiring the gem's name to update the require
to a path inside the gem.
For example, if a gem had:
This has to change to:
Not a huge code change, but needs to be done everywhere.
Changes
Applies the above suggestion to code that lives inside this repository.
Considerations
One issue is that this kind of violates the namespace expectation of a
gem called
refinerycms-core, which should havelib/refinerycms/core.rb.Perhaps this is worth a lot of consideration as an alternative.
Resolves #3464