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 and update sandbox script #169

Merged
merged 1 commit into from
Jul 21, 2021

Conversation

gsmendoza
Copy link
Contributor

@gsmendoza gsmendoza commented Jul 20, 2021

Goal

As a solidus_starter_frontend contributor
I want the gem's sandbox script to generate the app with the solidus_starter_frontend gem as an engine
So that the app would pick up changes I make to the app code

Background

Currently, the sandbox script both loads the gem and runs its generator. We want it to only the load the gem.

Previous implementations

Relevant links

#167 (comment) - discussion on having two strategies for generating the sandbox app: one with the Starter FE running as an engine and other where Starter FE is generated into the app.

Implementation

  • Point solidus gems in sandbox Gemfile to those in
    solidus_starter_frontend's Gemfile.

  • Fix: Install solidus using solidus:install instead of
    spree:install.

  • Fix: Pass --auto-run-migrations instead of --auto-accept to
    solidus:auth:install.

  • Remove solidus_starter_frontend call.

  • Call bundle exec rails g solidus_starter_frontend:install.

Demo

https://www.loom.com/share/1c7c4ff55baf4d8d8404a77f8520ca30

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.

Goal
----

As a solidus_starter_frontend contributor
I want the gem's sandbox script to generate the app with the solidus_starter_frontend gem as an engine
So that the app would pick up changes I make to the app code

Background
----------

Currently, the sandbox script both loads the gem and runs its generator. We want it to only the load the gem.

Previous implementations
------------------------

* [Document how to run the extension as an engine in a Rails app](a4600e2) - This commit documents how to install solidus_starter_frontend as an engine.
* Fix sandbox script #166 - This PR fixed the script to run `solidus_starter_frontend`  as a generator instead of as a gem.

Relevant links
--------------

#167 (comment) - discussion on having two strategies for generating the sandbox app: one with the Starter FE running as an engine and other where Starter FE is generated into the app.

Implementation
--------------

* Point solidus gems in sandbox Gemfile to those in
solidus_starter_frontend's Gemfile.

* Fix: Install solidus using `solidus:install` instead of
`spree:install`.

* Fix: Pass `--auto-run-migrations` instead of `--auto-accept` to
`solidus:auth:install`.

* Remove `solidus_starter_frontend` call.

* Call `bundle exec rails g solidus_starter_frontend:install`.
@gsmendoza gsmendoza marked this pull request as ready for review July 20, 2021 09:51
unbundled bundle exec rails generate solidus:auth:install --auto-accept
unbundled bundle exec rails generate ${extension_name}:install --auto-accept
unbundled bundle exec rails generate solidus:auth:install --auto-run-migrations
unbundled bundle exec rails g solidus_starter_frontend:install
Copy link
Contributor Author

Choose a reason for hiding this comment

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

An alternative to running the solidus_starter_frontend:install install command is to add the assets directly to app/views/spree/layouts/spree_application.html.erb and then strip them from the layout when running the generator. Let me know if you prefer this alternative.

Copy link
Member

Choose a reason for hiding this comment

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

Not sure to get what you mean here, do you have an example?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kennyadsl I was easier to create a separate PR implementing that change :) Please see #170. Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

I'd just go with this PR's route for now.

@gsmendoza gsmendoza mentioned this pull request Jul 21, 2021
6 tasks
@kennyadsl kennyadsl merged commit 8dd502a into master Jul 21, 2021
@kennyadsl kennyadsl deleted the gsmendoza/fix-and-update-sandbox-script branch July 21, 2021 10:16
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.

2 participants