Skip to content

Do not reload routes before application startup#255

Merged
kennyadsl merged 1 commit intosolidusio:mainfrom
mamhoff:do-not-reload-routes
Jan 7, 2025
Merged

Do not reload routes before application startup#255
kennyadsl merged 1 commit intosolidusio:mainfrom
mamhoff:do-not-reload-routes

Conversation

@mamhoff
Copy link
Copy Markdown
Contributor

@mamhoff mamhoff commented Dec 21, 2024

This devise.rb initializer is run for most stores, and by setting reload_routes to false, we can avoid Devise reloading the routes in an initializer. Rails will do this just fine, but not in the initializer chain.

This devise.rb initializer is run for most stores, and by setting
`reload_routes` to `false`, we can avoid Devise reloading the routes in
an initializer. Rails will do this just fine, but not in the initializer
chain.
@tvdeyen tvdeyen requested a review from a team January 6, 2025 14:39
@tvdeyen
Copy link
Copy Markdown
Member

tvdeyen commented Jan 6, 2025

@solidusio/core-team can I get a second review?

@kennyadsl
Copy link
Copy Markdown
Member

Thanks Martin, what problem does this change solve exactly?

@mamhoff
Copy link
Copy Markdown
Contributor Author

mamhoff commented Jan 7, 2025

It stops every store from loading the routes twice during startup.

@kennyadsl
Copy link
Copy Markdown
Member

And we have this problem on all extensions?

@mamhoff
Copy link
Copy Markdown
Contributor Author

mamhoff commented Jan 7, 2025

No, it's just this one, because it loads Devise, and that does the route reloading unless it's turned off. This was unearthed by the following issue in Solidus: solidusio/solidus#6038 - This user was running a store without solidus_auth_devise, thus didn't load the routes when Devise does, thus breaking solidus_admin. I think it's preferable for this extension to behave like any other authentication solution, and show us problems like this. (The actual error would be fixed by solidusio/solidus#6039).

@kennyadsl kennyadsl merged commit 1c3ee46 into solidusio:main Jan 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants