Skip to content
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.

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

Fix installation method as a gem executable #162

Closed

Conversation

waiting-for-dev
Copy link
Contributor

@waiting-for-dev waiting-for-dev commented Jun 11, 2021

This is a WIP that depends on changes that need to be integrated on solidus_support. In order to try it:

  • Check out this branch or your local repository.
  • Make sure you don't have other installation of solidus_starter_frontend: gem uninstall solidus_starter_frontend
  • Install the gem from this branch:
    • gem build solidus_starter_frontend.gemspec
    • gem install solidus_starter_frontend-0.1.0.gem
  • Create a rails app: rails new store --skip-javascript && cd store
  • Add solidus gems: bundle add solidus_core solidus_backend solidus_api solidus_sample
  • IMPORTANT: Add gem 'solidus_support', github: 'waiting-for-dev/solidus_support', branch: 'waiting-for-dev/change_engine_detection' to the Gemfile
  • Run bundle
  • Run bin/rails g solidus:install
  • Say y to the prompt suggesting solidus_auth_devise
  • Run solidus_starter_frontend
  • Run rails server bin/rails server
  • Go to http://localhost:3000 and check that the devise routes are working. E.g., http://localhost:3000/login.

Description

To support solidus_auth_devise, it needs to rely on a new version of
solidus_support that changes how it detects if a Solidus frontend is
available and the moment it adds the extensions' controller & view
directories to the Rails known paths.

Motivation and Context

The previous solidus_support version added the extensions' paths on
the engine's (in this case, solidus_auth_devise's engine) load time
and only when SolidusSupport#frontend_available? returned true.
Engines are loaded before the application code, so at that time, the
monkey patching of #frontend_available? that we were generating was
not at hand. The solution is two-fold: we add the paths on an
initializer on one side. However, at that moment, the application code
is neither available, so on the other side, we change
#frontend_available? to look for a
Rails.configuration.x.solidius.frontend_available setting that we add
to application.rb using the generator code. To understand the whole
picture, keep in mind that Rails.configuration is available on an
initializer, but it's not on an engine's load time.
On top of that, we need to update a couple of things on the generated code:

  • We need to reference Rails.root instead of Engine.root, as
    SolidusStarterFrontend::Engine is unavailable on this installation
    method.
  • We need to copy to application.rb the custom code on
    SolidusStarterFrontend::Engine for the same reason as above.

How Has This Been Tested?

Tested the installation method locally.

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

References #153 & #161

@@ -10,7 +10,8 @@ module AuthViews
protected

def configure_views
prepend_view_path Engine.root.join('lib', 'views', 'auth')
root = defined?(SolidusStarterFrontend::Engine) ? SolidusStarterFrontend::Engine.root : Rails.root
prepend_view_path root.join('lib', 'views', 'auth')
Copy link
Member

Choose a reason for hiding this comment

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

I think we can avoid this if we change

-    directory 'lib/views', 'lib/views'
+    directory 'lib/views', 'app/views'

This way all the views will be copied in the app directory, which is where people expect to find them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point!! I just updated it. I also had to add the exclude_pattern option in:

directory 'app', 'app', exclude_pattern: /auth_views/

@waiting-for-dev waiting-for-dev force-pushed the waiting-for-dev/fix_executable_installation branch 3 times, most recently from 9e7bfd6 to c455a2d Compare June 15, 2021 06:38
@@ -5,26 +5,39 @@ class SolidusStarterFrontendGenerator < Rails::Generators::Base

def install
# Copy directories
directory 'app', 'app'
directory 'lib/views', 'lib/views'
directory 'app', 'app', exclude_pattern: /auth_views/
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand why we need this part: exclude_pattern: /auth_views/. There is no auth_views folder in the app directory, isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's because of this file:

https://github.com/nebulab/solidus_starter_frontend/blob/waiting-for-dev/fix_executable_installation/app/controllers/concerns/solidus_starter_frontend/auth_views.rb

We don't need it, thanks to your suggestion to add the views directly into the application. However, it crashes because of Engine not being within the namespace if we copy it.

@@ -9,6 +9,10 @@ class Engine < Rails::Engine

engine_name 'solidus_starter_frontend'

initializer 'solidus_starter_frontend', before: 'solidus_support_frontend_paths' do
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to add solidus_support_frontend_paths here?

Copy link
Contributor Author

@waiting-for-dev waiting-for-dev Jun 15, 2021

Choose a reason for hiding this comment

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

Yes, for now, it's added on my fork https://github.com/waiting-for-dev/solidus_support/blob/bcda06b7ff436018b5a3eb2588ca55a7128a7d5a/lib/solidus_support/engine_extensions.rb#L85. For now, you need to manually add it to the Gemfile if you want to test this branch.

To support `solidus_auth_devise`, it needs to rely on a new version of
`solidus_support` that changes how it detects if a Solidus frontend is
available and the moment it adds the extensions' controller & view
directories to the Rails known paths.

The previous `solidus_support` version added the extensions' paths on
the engine's (in this case, `solidus_auth_devise`'s engine) load time
and only when `SolidusSupport#frontend_available?` returned `true`.
Engines are loaded before the application code, so at that time, the
monkey patching of `#frontend_available?` that we were generating was
not at hand. The solution is two-fold: we add the paths on an
`initializer` on one side. However, at that moment, the application code
is neither available, so on the other side, we change
`#frontend_available?` to look for a
`Rails.configuration.x.solidius.frontend_available` setting that we add
to `application.rb` using the generator code. To understand the whole
picture, keep in mind that `Rails.configuration` is available on an
initializer, but it's not on an engine's load time.
On top of that, we need to update a couple of things on the generated code:

- We need to reference `Rails.root` instead of `Engine.root`, as
  `SolidusStarterFrontend::Engine` is unavailable on this installation
  method.
- We need to copy to `application.rb` the custom code on
  `SolidusStarterFrontend::Engine` for the same reason as above.
@waiting-for-dev
Copy link
Contributor Author

waiting-for-dev commented Sep 15, 2021

@gsmendoza I think we can close this PR as you took another direction, right?

@gsmendoza
Copy link
Contributor

@waiting-for-dev Yes. Thanks for the work here! It helped me figure out how to move forward with the project :)

@gsmendoza gsmendoza closed this Sep 15, 2021
@waiting-for-dev waiting-for-dev deleted the waiting-for-dev/fix_executable_installation branch September 16, 2021 02:48
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.

3 participants