Load reqs with zeitwerk (drop ruby 2.3 support)#209
Conversation
mcelicalderon
left a comment
mcelicalderon
left a comment
There was a problem hiding this comment.
Looks good, @00dav00! My main concern is the versions we would need to drop support for. Ruby version and not sure if some versions of Rails. Ruby version might be fine at > 2.4.4, but not sure about rails if there's something around that
| spec.add_dependency 'devise_token_auth', '>= 0.1.43', '< 2.0' | ||
| spec.add_dependency 'graphql', '>= 1.8', '< 1.13.0' | ||
| spec.add_dependency 'rails', '>= 4.2', '< 6.2' | ||
| spec.add_dependency 'zeitwerk', '~> 2.2' |
There was a problem hiding this comment.
This would require dropping support for ruby 2.3 https://github.com/fxn/zeitwerk/blob/v2.2.0/zeitwerk.gemspec#L20
There was a problem hiding this comment.
Just curious, why ~> 2.2? Is it necessary to specify the version?
There was a problem hiding this comment.
No particular reason, let me investigate a bit more about backwards compatibility in order to move this forward 👍
| module ActionDispatch::Routing | ||
| class Mapper | ||
| module GraphqlDevise | ||
| module RoutesMounter |
There was a problem hiding this comment.
Perhaps RouteMounter. Or maybe GraphqlDevise::MountsRoute
There was a problem hiding this comment.
RouteMounter sounds good to me 👍
|
|
||
| loader.setup | ||
|
|
||
| ActionDispatch::Routing::Mapper.send(:include, GraphqlDevise::RoutesMounter) |
There was a problem hiding this comment.
Why send instead of
ActionDispatch::Routing::Mapper.include(GraphqlDevise::RoutesMounter)| require 'graphql' | ||
| require 'devise_token_auth' | ||
| require 'zeitwerk' | ||
| require_relative '../app/models/graphql_devise/concerns/model' |
There was a problem hiding this comment.
If we are going to explicitly require these, we should probably move them to lib so they are not autoloaded by the rails engine
There was a problem hiding this comment.
I was planning to add new concerns that comply with Zeitwerk name constraints inside of lib and add deprecation warnings to the current constraints (for instance GraphqlDevise::Authenticable vs GraphqlDevise::Concerns::Model). Let me know if you still consider we should move them and I can address it in the next commits.
There was a problem hiding this comment.
Yeah, but I think either way, we should probably move them away from the app directory as the engine will autoload that path too.
00dav00
force-pushed
the
load_reqs_with_zeitwerk
branch
from
7c7c86a to
2abee77
Compare
mcelicalderon
left a comment
mcelicalderon
left a comment
There was a problem hiding this comment.
Looks good, @00dav00! Left some comments. Specially make sure we avoid code duplication on deprecated concerns. Also you are missing to do the same you did for the model concern for the controller concern (set_user_by_token). The legacy concerns should only include the new concern and show a deprecation warning. ControllerMethods is an internal concern, no need to deprecate that one, simply add the new one to the base mutation and resolver as you already did.
| extend ActiveSupport::Concern | ||
|
|
||
| included do | ||
| ActiveSupport::Deprecation.warn(<<-DEPRECATION.strip_heredoc, caller) |
There was a problem hiding this comment.
This is an internal concern, no need to deprecate. Simply use the new one in the base resolvers and mutations
|
|
||
| included do | ||
| include DeviseTokenAuth::Concerns::User | ||
| include AdditionalModelMethods |
There was a problem hiding this comment.
You can simply include the new concern and show deprecation warning on this one, no need to copy all the concern's code.
| saved | ||
| else | ||
| raise( | ||
| GraphqlDevise::Error, |
There was a problem hiding this comment.
Error is such a generic name that I think it doesn't harm to be explicit about which error
mcelicalderon
changed the title
2 participants
This PR uses (Zeitwerk)[https://github.com/fxn/zeitwerk] to loads the gem files.
Drops support for ruby 2.3
TODO
requireandrequire_relativecalls