Fix expectations about solidus_auth_devise order in the Gemfile #4465
Conversation
solidus_frontend tries to load a partial from solidus_auth_devise [1] [2]. However, it provided a fallback [3] when the latter was missing. Unfortunately, it worked with the assumption that solidus_auth_devise went below solidus_frontend in the Gemfile. Otherwise, the fallback template took precedence. We're now removing the fallback template and rendering the template only when it's present. This way, it makes no critical which gem goes first. [1] - https://github.com/solidusio/solidus_frontend/blob/dae732ed7442a755cc0426cd2afa6ac662de7105/app/views/spree/shared/_nav_bar.html.erb#L4 [2] - https://github.com/solidusio/solidus_auth_devise/blob/bfbf616ef625aa51c6b74040d96998082f736e13/lib/views/frontend/spree/shared/_login_bar_items.html.erb [3] - https://github.com/solidusio/solidus_frontend/blob/dae732ed7442a755cc0426cd2afa6ac662de7105/app/views/spree/shared/_login_bar_items.html.erb
solidus_backend tries to load a partial from solidus_auth_devise [1] [2]. However, it provided a fallback [3] when the latter was missing. Unfortunately, it worked with the assumption that solidus_auth_devise went below solidus_backend in the Gemfile. Otherwise, the fallback template took precedence. We're now renaming the fallback partial and branching to the one in solidus_auth_devise when present. This way, it makes no critical which gem goes first. [1] - https://github.com/solidusio/solidus/blob/a22723079f88387f53f72b503db875b9d97aa3f5/backend/app/views/spree/admin/shared/_navigation.html.erb#L10 [2] - https://github.com/solidusio/solidus_auth_devise/blob/master/lib/views/backend/spree/admin/shared/_navigation_footer.html.erb [3] - https://github.com/solidusio/solidus/blob/master/backend/app/views/spree/admin/shared/_navigation_footer.html.erb
|
I don't know if I like not assuming that For the frontend, it is another story. If we remove all the frontend code from the extension, the order should no longer matter. What about changing the |
I'm not sure I follow. With these changes, solidus_backend can live both before and after solidus_auth_devise in the Gemfile 🤔 |
Yes, but my point is that there's no need to optimize |
As far as I can see, Both situations happen after all the gems have been required. It should be independent of their order. As per the implicit work from solidus_support, we fixed the dependency on the Gemfile order in solidusio/solidus_support@202e77c. If we're missing something, I think we should try fixing it as this PR does. IMHO, requiring the gems to be declared in any particular order is unacceptable. Users could find a tough time debugging a problem with a root outside of the code space, so to speak. FWIW, the work's origin here is the changes that we will submit to support the installation of solidus_starter_frontend in the Solidus installer. The current installer will add solidus_auth_devise after Solidus. But we must run the new frontend installation script after installing Solidus. When users choose it, we need to remove the solidus meta-gem and replace it with solidus_core, solidus_backend, solidus_api & solidus_sample, so they don't pull solidus_frontend. As built-in bundler utilities that modify the Gemfile work by appending, the process switches the order and puts solidus_backend after solidus_auth_devise. |
|
Got it, I think it's a matter of choosing if we want to encourage adding extensions wherever in the Gemfile, or only after the core gems. Let me explain what I mean: if a new developer ends up the installation process with a Gemfile that looks like this: where do we think they are going to add the other extensions later? Is it acceptable at the moment? Do the other extensions currently support living before the core gem? I think they don't (even if I'm not 100% sure).
Thanks for adding more context. As a curiosity, did you think about adding |
I think that's not our call. Users should be able to add them wherever they want. The Gemfile order should not determine the resulting runtime. If we look at the docs about what is a Gemfile on Bundler's site, they don't mention that the gems are required in any particular order. Therefore, I don't think we should treat it as something deterministic. They already changed how they're required in the past. True, now that happens in declaration order, as it's the most sensible default (as we can see here), but it's not part of the documentation. That The Gemfile order should be more seen as the result of a style guide. For instance, Rubocop has the
Yeah, that would be possible, but it'll result in a less linear installation flow and, sometimes, an extra |
|
You got me, this is the correct way to go. If other extensions won't work listed before the core gems, we should fix that somewhere else. |
Yeah, I think that's the way to go. Thanks for bearing with me 🙂 |
|
Looking good, thank you! |
waiting-for-dev
deleted the
waiting-for-dev/fix_solidus_auth_devise_gemfile_order
branch
Summary
solidus_frontend tries to load a partial from solidus_auth_devise. However, it provided a fallback when the latter was missing.
The same is true in solidus_backend; it also tries to load a partial from solidus_auth_devise, but provides its fallback
Unfortunately, that worked with the assumption that solidus_auth_devise went below solidus_frontend and solidus_backend respectively in the Gemfile. Otherwise, the fallback templates took precedence.
For solidus_frontend, we're removing the placeholder template and rendering the one from solidus_auth_devise only when present. Similarly, for solidus_backend we're renaming the fallback template and rendering the one from solidus_auth_devise when present.
Checklist
Check out our PR guidelines for more details.
The following are mandatory for all PRs:
The following are not always needed (
cross them outif they are not):[ ] I have added automated tests to cover my changes.[ ] I have attached screenshots to demo visual changes.[ ] I have opened a PR to update the guides.[ ] I have updated the readme to account for my changes.