Frontend installers with app-templates #4629

elia · 2022-09-15T14:21:17Z

Summary

This change makes it trivial to add/remove/change frontend or to pass a custom template to be used. This among other use-cases can become handy for testing out branches of solidus_starter_frontend without touching the code of the installer.

It's closer to what you would expect coming from rails new options.
--frontend=none was fixed and now properly removes solidus_frontend.
The bundler splitter was compacted into a single file and always adds solidus_frontend leaving the choice to other templates if to keep it.
A job testing the installer has been added to the CI

Checklist

Check out our PR guidelines for more details.

The following are mandatory for all PRs:

I have written a thorough PR description.
I have kept my commits small and atomic.
I have used clear, explanatory commit messages.

The following are not always needed (~~cross them out~~ if 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.

forkata

Nice work here @elia! This is probably not in scope for this PR, but I am curious if we have plans to enable similar behaviour for the dummy app generator as well. We've run into some challenges adding the solidus_starter_frontend to a dummy app (for extension testing) post generation due to the fact that there is no Gemfile.

elia · 2022-09-16T15:30:49Z

@forkata that's a good point, can you please help give me the ropes on how to replicate that issue?

(on the same topic I'm also experimenting with using app-templates for more stuff in the installer and for turning some extensions into a sort of recipes, I'll send another PR for that if I end up with something viable)

waiting-for-dev · 2022-09-19T04:45:06Z

bin/sandbox

@@ -3,6 +3,10 @@
 # Used in the sandbox rake task in Rakefile

 set -e
+if [ ! -z $DEBUG ]


Not a big deal, but I think it won't catch when $DEBUG is set to the empty string:

> [ ! -z '' ] > echo $? 1

I've usually seen to check for the presence of an env variable is if [ -z ${var+x} ], which returns false if the variable is set to the empty string or any other string.

I think it's ok to assume it will have some contents when set, that said maybe I'm doing some bash mistake here:

solidus:elia/frontend-templates ⤑ bash -l -c 'DEBUGz="asdf" test -z ${DEBUG+x}; echo $?' 0 solidus:elia/frontend-templates ⤑ bash -l -c 'DEBUG="asdf" test -z ${DEBUG+x}; echo $?' 0 solidus:elia/frontend-templates ⤑ bash -l -c 'DEBUG="" test -z ${DEBUG+x}; echo $?' 0

It looks like the parameter expansion is evaluated before the command, so $DEBUG is never defined at that point. But if you run it as part of a script, it'll work.

Thanks for the explanation, let me know if you would still prefer to support an empty string there

I think it makes sense to support them. I know the code looks weirder, but it's not our fault that bash is bizarre 😄 Up to you! 🙂

Done! TIL 🙏

bin/sandbox

core/lib/generators/solidus/install/install_generator.rb

core/lib/generators/solidus/install/frontend_templates/solidus_frontend.rb

waiting-for-dev · 2022-09-19T05:07:26Z

core/lib/generators/solidus/install/frontend_templates/break_down_solidus_gem.rb

+
+# `solidus_frontend` is not sourced from rubygems if the solidus gem was not.
+github_solidus_frontend = '--github solidusio/solidus_frontend' unless solidus.source.is_a? Bundler::Source::Rubygems
+bundle_command("add solidus_frontend --skip-install #{github_solidus_frontend}")


Instead of adding and later removing, why not add only when solidus_frontend is chosen? 🤔

This way we have a common baseline that is supposedly equivalent to the original but with exploded list of components, up to the scripts that don't require solidus_frontend to remove it.

Given that is the component we want to deprecate and that I wouldn't say undoing an action (or definition of action) is a good pattern for expansion, I would still have it only where it's needed. But not a blocker. I'll be fine with whatever you decide 🙂

Done, now the starter_frontend just adds the gem, and the break-down recipe doesn't do that anymore.

waiting-for-dev · 2022-09-19T05:09:24Z

core/lib/generators/solidus/install/frontend_templates/break_down_solidus_gem.rb

+ solidus
+])
+
+# `solidus_frontend` is not sourced from rubygems if the solidus gem was not.


I'm not sure I get it. Could you explain it to me? 🙏

The idea is that if you have added solidus from rubygems you want to take the latest compatible solidus_frontend from rubygems as well.

On the contrary if you added it from git/github/path then it's better if we add it from GitHub as well.

You think it's fine to always get it from Rubygems?

Oh, yeah, it makes sense.

However, we should skip adding it when it's already in the Gemfile, right?

Adding solidus_frontend was removed from this app-template.

waiting-for-dev

Thanks for working on it, @elia! There are very nice improvements here.

However, I think something is wrong when installing solidus_starter_frontend, as when I go to http:https://localhost:3000 I see Rails' welcome page instead. It's quite possible the error is on solidus_starter_frontend's side, but we need to investigate it.

I see the benefits of using rails templates for the frontend installers, but I'm afraid we lose proper encapsulation in this way. For instance, we're now duplicating how we check for gems being present in the Gemfile and, more importantly, we no longer respect --auto-accept when delegating to the solidus_frontend installer. What do you think about it?

elia · 2022-09-19T09:28:25Z

Thanks for the review @waiting-for-dev 🙏

I solved the the issue with the welcome page, it was applying the engine route to the Core routes block that is prepended by the SSF. I added running of the sandbox specs in the solidus-installer CI job, still a sort of smoke test but should be able to catch similar errors in the future.

Also now it forwards both --auto-accept (from options[:auto_accept]) and --auto-run-migrations (from options[:migrate]).

Use `bundle info [gemname]` to see where a bundled gem is installed.
    generate    solidus_frontend:install
       rails    generate solidus_frontend:install --auto-accept --auto-run-migrations
      create  config/initializers/solidus_frontend.rb

waiting-for-dev · 2022-09-20T07:55:43Z

.circleci/config.yml

+ command: |
+ cd /tmp/my_app
+ bundle add solidus --git "file:https://$(ruby -e"puts File.expand_path ENV['CIRCLE_WORKING_DIRECTORY']")"
+ export SKIP_SOLIDUS_BOLT='true' # workaround for solidus_frontend not respecting --auto-accept when asking about bolt


Now that we're respecting --auto-accept, we can:

Remove the comment and keep skipping solidus_bolt.

Remove the line and install solidus_bolt.

~~Opted for removing the line altogether, we should test things as close as possible to the final usage~~

Tried removing the line, but fails with this error, somehow I think bundle_cleanly in the solidus_frontend gem has a bad interaction with the main script, so I'm adding it back, but updating the comment.

Using solidus 3.3.0.alpha from file:https:///home/circleci/solidus (at elia/frontend-templates@34468ee) generate solidus_frontend:install rails generate solidus_frontend:install --auto-accept --auto-run-migrations create config/initializers/solidus_frontend.rb exist app/assets/images create vendor/assets/javascripts/spree/frontend create vendor/assets/stylesheets/spree/frontend create vendor/assets/images/spree/frontend create vendor/assets/javascripts/spree/frontend/all.js create vendor/assets/stylesheets/spree/frontend/all.css gemfile solidus_bolt generate solidus_bolt:install --auto-run-migrations rails generate solidus_bolt:install --auto-run-migrations Could not find solidus_bolt-0.7.2, coffee-rails-5.0.0, httparty-0.20.0, multi_json-1.15.0, omniauth-bolt-0.3.0, solidus_social-1.5.0, tweetnacl-1.0.0, coffee-script-2.4.1, multi_xml-0.6.0, omniauth-oauth-1.2.0, oa-core-0.3.2, omniauth-2.1.0, omniauth-facebook-9.0.0, omniauth-github-2.0.0, omniauth-google-oauth2-0.8.0, omniauth-rails_csrf_protection-1.0.1, coffee-script-source-1.12.2, oauth-1.1.0, hashie-5.0.0, rack-protection-2.2.2, omniauth-oauth2-1.7.3, jwt-2.5.0, oauth-tty-1.0.5, snaky_hash-2.0.0, version_gem-1.1.1, oauth2-2.0.9, faraday-2.5.2, faraday-net_http-3.0.0, ruby2_keywords-0.0.5 in any of the sources Run `bundle install` to install missing gems. Exited with code exit status 1

https://app.circleci.com/pipelines/github/nebulab/solidus/391/workflows/05b8d862-a54b-4c2d-a00e-ce9304e8c7fe/jobs/4875

elia

@waiting-for-dev thanks for the reviews, I should have addressed everything 🙏

elia · 2022-09-21T13:48:08Z

.circleci/config.yml

+ command: |
+ cd /tmp/my_app
+ bundle add solidus --git "file:https://$(ruby -e"puts File.expand_path ENV['CIRCLE_WORKING_DIRECTORY']")"
+ export SKIP_SOLIDUS_BOLT='true' # workaround for solidus_frontend not respecting --auto-accept when asking about bolt


~~Opted for removing the line altogether, we should test things as close as possible to the final usage~~

Tried removing the line, but fails with this error, somehow I think bundle_cleanly in the solidus_frontend gem has a bad interaction with the main script, so I'm adding it back, but updating the comment.

Using solidus 3.3.0.alpha from file:https:///home/circleci/solidus (at elia/frontend-templates@34468ee) generate solidus_frontend:install rails generate solidus_frontend:install --auto-accept --auto-run-migrations create config/initializers/solidus_frontend.rb exist app/assets/images create vendor/assets/javascripts/spree/frontend create vendor/assets/stylesheets/spree/frontend create vendor/assets/images/spree/frontend create vendor/assets/javascripts/spree/frontend/all.js create vendor/assets/stylesheets/spree/frontend/all.css gemfile solidus_bolt generate solidus_bolt:install --auto-run-migrations rails generate solidus_bolt:install --auto-run-migrations Could not find solidus_bolt-0.7.2, coffee-rails-5.0.0, httparty-0.20.0, multi_json-1.15.0, omniauth-bolt-0.3.0, solidus_social-1.5.0, tweetnacl-1.0.0, coffee-script-2.4.1, multi_xml-0.6.0, omniauth-oauth-1.2.0, oa-core-0.3.2, omniauth-2.1.0, omniauth-facebook-9.0.0, omniauth-github-2.0.0, omniauth-google-oauth2-0.8.0, omniauth-rails_csrf_protection-1.0.1, coffee-script-source-1.12.2, oauth-1.1.0, hashie-5.0.0, rack-protection-2.2.2, omniauth-oauth2-1.7.3, jwt-2.5.0, oauth-tty-1.0.5, snaky_hash-2.0.0, version_gem-1.1.1, oauth2-2.0.9, faraday-2.5.2, faraday-net_http-3.0.0, ruby2_keywords-0.0.5 in any of the sources Run `bundle install` to install missing gems. Exited with code exit status 1

https://app.circleci.com/pipelines/github/nebulab/solidus/391/workflows/05b8d862-a54b-4c2d-a00e-ce9304e8c7fe/jobs/4875

elia · 2022-09-21T14:22:42Z

bin/sandbox

@@ -3,6 +3,10 @@
 # Used in the sandbox rake task in Rakefile

 set -e
+if [ ! -z $DEBUG ]


Done! TIL 🙏

elia · 2022-09-21T14:23:49Z

core/lib/generators/solidus/install/frontend_templates/break_down_solidus_gem.rb

+
+# `solidus_frontend` is not sourced from rubygems if the solidus gem was not.
+github_solidus_frontend = '--github solidusio/solidus_frontend' unless solidus.source.is_a? Bundler::Source::Rubygems
+bundle_command("add solidus_frontend --skip-install #{github_solidus_frontend}")


Done, now the starter_frontend just adds the gem, and the break-down recipe doesn't do that anymore.

elia · 2022-09-21T14:24:33Z

core/lib/generators/solidus/install/frontend_templates/break_down_solidus_gem.rb

+ solidus
+])
+
+# `solidus_frontend` is not sourced from rubygems if the solidus gem was not.


Adding solidus_frontend was removed from this app-template.

benjaminwil · 2022-09-21T22:41:25Z

@elia:

@forkata that's a good point, can you please help give me the ropes on how to replicate that issue?

We just created reproduction instructions and started a discussion for it: #4634

waiting-for-dev · 2022-09-22T07:41:51Z

bin/sandbox

@@ -3,6 +3,7 @@
 # Used in the sandbox rake task in Rakefile

 set -e
+test -z "${DEBUG+empty_string}" && set -x


Careful! Length is zero (-z) when $DEBUG is not set, so it's actually doing the opposite that we want.

🤦‍♂️ thanks, fixed

Mostly fixing the double space.

Rather than custom `puts` and `Kernel#\`` use helpers like `rake`, `route`, and `say_status`.

Move it before installing the frontend so it's not hindered by routes added to the Core engine, which are confusing for the helper.

Persisting the solidus version to a file was there just to support stoplight. The was job added in 334e773. Spotlight was removed in 8f499b2.

Includes a smoke test ensuring the application starts and has the expected copy on the home page.

…lpers

Generators supporting those options will now be able to respect the intent given in the original install command.

That handling was added at a time in which the sandbox couldn't function without it. Now it just confuses the frontend installers skipping the solidus_auth_devise plugin installation if it is already in the Gemfile.

This change makes it trivial to add/remove/change frontend or to pass a custom template to be used. This among other use-cases can become handy for testing out branches of solidus_starter_frontend without touching the code of the installer. It's also closer to what you would expect coming from `rails new` options. In the process `--frontend=none` was fixed and now properly removes `solidus_frontend`. The bundler splitter was compacted into a single file and always adds `solidus_frontend` leaving the choice to other templates if to keep it.

waiting-for-dev

Thanks, @elia! You're great.

kennyadsl

Wow, thanks Elia!

github-actions bot added the changelog:solidus_core Changes to the solidus_core gem label Sep 15, 2022

elia force-pushed the elia/frontend-templates branch 4 times, most recently from 21cacdb to 7271b2c Compare September 15, 2022 16:40

forkata reviewed Sep 15, 2022

View reviewed changes

elia force-pushed the elia/frontend-templates branch 4 times, most recently from 0b5136f to bee4078 Compare September 16, 2022 15:24

elia force-pushed the elia/frontend-templates branch from 35fe4ba to 173f14d Compare September 16, 2022 16:35

elia marked this pull request as ready for review September 16, 2022 16:45

elia force-pushed the elia/frontend-templates branch from 173f14d to cc2dbd0 Compare September 16, 2022 17:52

waiting-for-dev reviewed Sep 19, 2022

View reviewed changes

bin/sandbox Outdated Show resolved Hide resolved

waiting-for-dev reviewed Sep 19, 2022

View reviewed changes

core/lib/generators/solidus/install/install_generator.rb Show resolved Hide resolved

waiting-for-dev reviewed Sep 19, 2022

View reviewed changes

core/lib/generators/solidus/install/frontend_templates/solidus_frontend.rb Outdated Show resolved Hide resolved

waiting-for-dev reviewed Sep 19, 2022

View reviewed changes

waiting-for-dev suggested changes Sep 19, 2022

View reviewed changes

waiting-for-dev requested a review from a team September 19, 2022 05:26

elia force-pushed the elia/frontend-templates branch 2 times, most recently from f4c69f0 to 29fc8f4 Compare September 19, 2022 09:24

elia requested a review from waiting-for-dev September 19, 2022 09:28

elia force-pushed the elia/frontend-templates branch 2 times, most recently from ab01d1e to e91118b Compare September 19, 2022 10:47

elia marked this pull request as draft September 19, 2022 11:51

elia force-pushed the elia/frontend-templates branch from e91118b to fd64209 Compare September 19, 2022 13:23

waiting-for-dev reviewed Sep 20, 2022

View reviewed changes

elia force-pushed the elia/frontend-templates branch 5 times, most recently from 34468ee to 3caefd0 Compare September 21, 2022 14:29

elia commented Sep 21, 2022

View reviewed changes

elia requested a review from waiting-for-dev September 21, 2022 14:44

waiting-for-dev reviewed Sep 22, 2022

View reviewed changes

elia added 11 commits September 22, 2022 10:17

Streamline conditional appended to a heredoc

89e8689

Mostly fixing the double space.

Fix forwarding arguments to solidus:install in bin/sandbox

4a650ea

Support debug output with the DEBUG env var in bin/sandbox

c2a1089

Install generator cleanup

098aa03

Rather than custom `puts` and `Kernel#\`` use helpers like `rake`, `route`, and `say_status`.

Use the standard route helper for installing routes

ef36cd9

Move it before installing the frontend so it's not hindered by routes added to the Core engine, which are confusing for the helper.

Remove stoplight support from the CI config

39d16c7

Persisting the solidus version to a file was there just to support stoplight. The was job added in 334e773. Spotlight was removed in 8f499b2.

Add a CI job for testing the installer with different options

ee5dfca

Includes a smoke test ensuring the application starts and has the expected copy on the home page.

Switch the install generator to AppBase to gain app:template style he…

0bd55c7

…lpers

Forward :auto_accept and :migrate options to generator sub-commands

67c80b8

Generators supporting those options will now be able to respect the intent given in the original install command.

elia force-pushed the elia/frontend-templates branch from 3caefd0 to 76665ec Compare September 22, 2022 08:17

elia requested a review from waiting-for-dev September 22, 2022 08:31

Add limited options to validate user input for the frontend

64b05a9

waiting-for-dev approved these changes Sep 23, 2022

View reviewed changes

kennyadsl approved these changes Sep 23, 2022

View reviewed changes

kennyadsl merged commit 992b288 into solidusio:master Sep 23, 2022

kennyadsl deleted the elia/frontend-templates branch September 23, 2022 14:34

elia mentioned this pull request Sep 23, 2022

Fix the CI gem cache for the solidus installer job #4638

Merged

7 tasks

waiting-for-dev added the type:enhancement Proposed or newly added feature label Sep 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Frontend installers with app-templates #4629

Frontend installers with app-templates #4629

elia commented Sep 15, 2022 •

edited

Loading

forkata left a comment

elia commented Sep 16, 2022

waiting-for-dev Sep 19, 2022

elia Sep 19, 2022

waiting-for-dev Sep 20, 2022

elia Sep 20, 2022

waiting-for-dev Sep 20, 2022

elia Sep 21, 2022

waiting-for-dev Sep 19, 2022

elia Sep 19, 2022

waiting-for-dev Sep 20, 2022

elia Sep 21, 2022

waiting-for-dev Sep 19, 2022

elia Sep 19, 2022

waiting-for-dev Sep 20, 2022

elia Sep 21, 2022

waiting-for-dev left a comment

elia commented Sep 19, 2022

waiting-for-dev Sep 20, 2022

elia Sep 21, 2022

elia left a comment

elia Sep 21, 2022

elia Sep 21, 2022

elia Sep 21, 2022

elia Sep 21, 2022

benjaminwil commented Sep 21, 2022

waiting-for-dev Sep 22, 2022 •

edited

Loading

elia Sep 22, 2022

waiting-for-dev left a comment

kennyadsl left a comment

Frontend installers with app-templates #4629

Frontend installers with app-templates #4629

Conversation

elia commented Sep 15, 2022 • edited Loading

Summary

Checklist

forkata left a comment

Choose a reason for hiding this comment

elia commented Sep 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waiting-for-dev left a comment

Choose a reason for hiding this comment

elia commented Sep 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjaminwil commented Sep 21, 2022

waiting-for-dev Sep 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waiting-for-dev left a comment

Choose a reason for hiding this comment

kennyadsl left a comment

Choose a reason for hiding this comment

elia commented Sep 15, 2022 •

edited

Loading

waiting-for-dev Sep 22, 2022 •

edited

Loading