Skip to content

Use optimized loading of default configuration for test / development environments #284

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
+28 −24
Open

Use optimized loading of default configuration for test / development environments #284

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
278e499
Update devise-jwt.gemspec
chaadow Aug 16, 2024
051eb59
Use optimized loading of configuration
chaadow Apr 13, 2025
1b630ef
Merge branch 'patch-1'
chaadow May 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion devise-jwt.gemspec
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Copy link
Contributor Author

@chaadow chaadow May 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

spec.add_dependency 'warden-jwt_auth', '~> 0.10'

spec.add_development_dependency "bundler", "> 1"
Expand Down
13 changes: 1 addition & 12 deletions lib/devise/jwt/railtie.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,7 @@ class Railtie < Rails::Railtie
initializer 'devise-jwt-middleware' do |app|
app.middleware.use Warden::JWTAuth::Middleware

config.after_initialize do
Rails.application.reload_routes!

config.after_routes_loaded do
Warden::JWTAuth.configure do |config|
defaults = DefaultsGenerator.call

Expand All @@ -21,15 +19,6 @@ class Railtie < Rails::Railtie
config.revocation_strategies = defaults[:revocation_strategies]
end
end

ActiveSupport::Reloader.to_prepare do
Warden::JWTAuth.configure do |config|
defaults = DefaultsGenerator.call

config.mappings = defaults[:mappings]
config.revocation_strategies = defaults[:revocation_strategies]
end
end
end
end
end
Expand Down
2 changes: 2 additions & 0 deletions spec/fixtures/rails_app/Gemfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ gem 'rails'
gem 'sqlite3'
# Use Puma as the app server
gem 'puma'

gem 'devise', github: 'heartcombo/devise', branch: 'main'
Copy link
Contributor Author

@chaadow chaadow May 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

# Build JSON APIs with ease. Read more: https://github.com/rails/jbuilder
# gem 'jbuilder', '~> 2.5'
# Use ActiveModel has_secure_password
Expand Down
29 changes: 18 additions & 11 deletions spec/fixtures/rails_app/Gemfile.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,20 @@
GIT
remote: https://github.com/heartcombo/devise.git
revision: fec67f98f26fcd9a79072e4581b1bd40d0c7fa1d
branch: main
specs:
devise (5.0.0.beta)
bcrypt (~> 3.0)
orm_adapter (~> 0.1)
railties (>= 6.0.0)
responders
warden (~> 1.2.3)

PATH
remote: ../../..
specs:
devise-jwt (0.12.1)
devise (~> 4.0)
devise (>= 4.0)
warden-jwt_auth (~> 0.10)

GEM
Expand Down Expand Up @@ -85,15 +97,9 @@ GEM
bigdecimal (3.1.9)
builder (3.3.0)
concurrent-ruby (1.3.5)
connection_pool (2.5.1)
connection_pool (2.5.3)
crass (1.0.6)
date (3.4.1)
devise (4.9.4)
bcrypt (~> 3.0)
orm_adapter (~> 0.1)
railties (>= 4.1.0)
responders
warden (~> 1.2.3)
drb (2.2.1)
dry-auto_inject (1.1.0)
dry-core (~> 1.1)
Expand Down Expand Up @@ -147,14 +153,14 @@ GEM
pp (0.6.2)
prettyprint
prettyprint (0.2.0)
psych (5.2.3)
psych (5.2.5)
date
stringio
puma (6.6.0)
nio4r (~> 2.0)
racc (1.8.1)
rack (3.1.13)
rack-session (2.1.0)
rack (3.1.14)
rack-session (2.1.1)
base64 (>= 0.1.0)
rack (>= 3.0.0)
rack-test (2.2.0)
Expand Down Expand Up @@ -225,6 +231,7 @@ PLATFORMS
ruby

DEPENDENCIES
devise!
devise-jwt!
puma
rails
Expand Down
6 changes: 6 additions & 0 deletions spec/spec_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,4 +30,10 @@

RSpec.configure do |config|
config.use_transactional_fixtures = true

# Make sure routes are loaded once before the test suite is run
# Since they are lazy loaded by default on Rails 8
config.before(:suite) do
Rails.application.try(:reload_routes_unless_loaded)
Copy link
Owner

@waiting-for-dev waiting-for-dev May 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chaadow , I'm wondering if users will also need to add this to their test suite to make them happy 🤔

Copy link
Contributor Author

@chaadow chaadow May 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🙏🏼

Copy link
Owner

@waiting-for-dev waiting-for-dev May 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

@waiting-for-dev waiting-for-dev May 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, for our test suite, we can probably just Rails.application.reload_routes_unless_loaded, right? Given we're using Rails 8.

Copy link
Contributor Author

@chaadow chaadow May 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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)

Copy link
Owner

@waiting-for-dev waiting-for-dev May 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

end
end