Use optimized loading of default configuration for test / development environments #284
Conversation
Devise started the development of devise 5 in its alpha phase. I believe it's safe to have a less strict on the dependency's version
There was a problem hiding this comment.
Pull Request Overview
This PR optimizes the loading of default configuration for test and development environments by adjusting when routes are loaded, thereby reducing unnecessary initialization overhead. The changes include a temporary hook in the test configuration and a replacement of the after_initialize block with an after_routes_loaded hook in the Railtie.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| spec/spec_helper.rb | Added a before hook to conditionally reload routes for tests. |
| lib/devise/jwt/railtie.rb | Replaced after_initialize with after_routes_loaded and removed redundant configuration reloading. |
| # TODO: Remove once Devise publishes a new version | ||
| # https://github.com/heartcombo/devise/issues/5705 | ||
| config.before do | ||
| Rails.application.try(:reload_routes_unless_loaded) |
There was a problem hiding this comment.
@chaadow , I'm wondering if users will also need to add this to their test suite to make them happy 🤔
There was a problem hiding this comment.
only those on Rails 8 and above ( because of lazy routes loading ), but most of us that are on rails 8 use the "main/master" version of devise because they have not cut a release yet :/
because if you look at the changelog: https://github.com/heartcombo/devise/blob/main/CHANGELOG.md
everything is "well supported" but it's still not released yet. and since this gem tests against the most recent version of devise ( on rubygems,) I did this workaround to make the tests pass!
Your call @waiting-for-dev 🙏🏼
There was a problem hiding this comment.
Gotcha. How about this: we could put it behind a configuration option that's disabled by default. That way, we wouldn't break anyone's existing test suites. But folks who are aware of these quirks could enable it and add the call to their spec_helper file. What do you think? It'd also be good to add a notice in the README. Once Devise releases a new version, we can clean it up.
There was a problem hiding this comment.
By the way, for our test suite, we can probably just Rails.application.reload_routes_unless_loaded, right? Given we're using Rails 8.
There was a problem hiding this comment.
@waiting-for-dev you are definitely right! I've just tested it and without it it fails. I also made a slight optimization to run it before(:suite)
There was a problem hiding this comment.
Hey @chaadow , just wanted to make sure you read this from above:
How about this: we could put it behind a configuration option that's disabled by default. That way, we wouldn't break anyone's existing test suites. But folks who are aware of these quirks could enable it and add the call to their spec_helper file. What do you think? It'd also be good to add a notice in the README. Once Devise releases a new version, we can clean it up.
No hurries on my end, but I wouldn't like you to be left waiting for me
| @@ -21,7 +21,7 @@ Gem::Specification.new do |spec| | |||
| spec.executables = spec.files.grep(%r{^exe/}) { |f| File.basename(f) } | |||
| spec.require_paths = ["lib"] | |||
|
|
|||
| spec.add_dependency 'devise', '~> 4.0' | |||
| spec.add_dependency 'devise', '>= 4.0' | |||
There was a problem hiding this comment.
I know i've made this change in this PR a while ago : ( #275 )
But I think we're finally ready to support devise 5. and seeing that they can cut a release at any moment ( based on the changelog i've sent below) @waiting-for-dev
| @@ -10,6 +10,8 @@ gem 'rails' | |||
| gem 'sqlite3' | |||
| # Use Puma as the app server | |||
| gem 'puma' | |||
|
|
|||
| gem 'devise', github: 'heartcombo/devise', branch: 'main' | |||
There was a problem hiding this comment.
@waiting-for-dev I thought that making the test suite run against the main branch of devise can help us catch any breaking change early on.
this goes hand in hand with spec.add_dependency 'devise', '>= 4.0'
otherwise it won't work @waiting-for-dev
Context
While investigating our big production rails application in development/test mode, i've realized that rails routes file was loaded on startup as well as all our devise models. It was thanks to this script in this conference.
the flow is the following :
1-
gem 'devise-jwt'inGemfile2- this registers a Railties that loads routes at initialization
3- then run a rails command such as
bin/rails consoleorbin/rails serverthat triggers the rails initializaiton process4- the hook registered by
devise-jwtis run at startup5- all routes are loaded even for
rails runnerorrails consoleorrails test( for unit tests that don't need routes to be loaded )this can speedup both development and test environement especially for big applications that have a lot of routes. and avoids loading ActiveRecord on startup when/if not needed
Solution
Rails recently added a hook called
after_routes_loadedthat triggers as the name suggests when the routes are loaded, and since devise-jwt needs the routes to be loaded in order to register the defaults for all devise models, this is I believe the perfect compromise for all environments ( test/dev and production )cc @waiting-for-dev