Fix: solidus:install adds the frontend assets even if the repo does not have solidus_frontend #4251
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ot have solidus_frontend Expected behavior ----------------- Given I have a Rails app And the Rails app only has the following Solidus gems: * solidus_core * solidus_backend * solidus_api * solidus_sample When I run `bin/rails generate solidus:install --auto-accept` Then I should see that the `vendor/assets/*/spree` directories do not include frontend directories with its assets. Actual behavior --------------- The `vendor/assets/*/spree` directories include the frontend directories with their assets. Cause ----- The `defined? Spree::Frontend || Rails.env.test?` in https://github.com/solidusio/solidus/blob/f524772df264fc4fffa971b992d2724f5060c1b7/core/lib/generators/solidus/install/install_generator.rb returns `"expression"`. Fix --- Wrapping the argument of the `defined?` call in a parenthesis fixes the issue, for example: ```rb defined?(Spree::Frontend) || Rails.env.test? ``` Similar issues -------------- The backend assets have the same issue and fix. Tested in --------- * Ruby 2.5.3p105 * Rails 6.1.4.4 * Solidus master (7e0a386)
jarednorman
approved these changes
Jan 27, 2022
|
Thanks @gsmendoza, and great work on your talk at the conference! |
waiting-for-dev
approved these changes
Jan 31, 2022
waiting-for-dev
deleted the
gsmendoza/eng-288-fix-solidusinstall-adds-the-frontend
branch
gsmendoza
added a commit
to solidusio/solidus_starter_frontend
that referenced
this pull request
Feb 21, 2022
Goal ---- We want the frontend assets and directories added to a Solidus app because Solidus extensions assume that these files and directories exist. The Solidus extensions typically link these pre-existing files to their assets. Expected behavior ----------------- Given I have a Solidus app without a frontend When I apply the SolidusStarterFrontend template to the app Then I should see the following frontend assets and directories added to the app: * `vendor/assets/images/spree/frontend` * `vendor/assets/javascripts/spree/frontend/all.js` * `vendor/assets/stylesheets/spree/frontend/all.css` Current behavior ---------------- SolidusStarterFrontend doesn't add the frontend assets and directories to the app's vendor directory. Bug cause --------- Prior to solidusio/solidus#4251, the Solidus Install generator was adding the frontend assets during installation even if the app didn't have `solidus_frontend` in its bundle. With the issue fixed, SolidusStarterFrontend has to be updated to add these frontend assets by itself.
2 tasks
gsmendoza
added a commit
to solidusio/solidus_starter_frontend
that referenced
this pull request
Feb 22, 2022
Goal ---- We want the frontend assets and directories added to a Solidus app because Solidus extensions assume that these files and directories exist. The Solidus extensions typically link these pre-existing files to their assets. Expected behavior ----------------- Given I have a Solidus app without a frontend When I apply the SolidusStarterFrontend template to the app Then I should see the following frontend assets and directories added to the app: * `vendor/assets/images/spree/frontend` * `vendor/assets/javascripts/spree/frontend/all.js` * `vendor/assets/stylesheets/spree/frontend/all.css` Current behavior ---------------- SolidusStarterFrontend doesn't add the frontend assets and directories to the app's vendor directory. Bug cause --------- Prior to solidusio/solidus#4251, the Solidus Install generator was adding the frontend assets during installation even if the app didn't have `solidus_frontend` in its bundle. With the issue fixed, SolidusStarterFrontend has to be updated to add these frontend assets by itself.
gsmendoza
added a commit
to solidusio/solidus_starter_frontend
that referenced
this pull request
Feb 22, 2022
Goal ---- We want the frontend assets and directories added to a Solidus app because Solidus extensions assume that these files and directories exist. The Solidus extensions typically link these pre-existing files to their assets. Expected behavior ----------------- Given I have a Solidus app without a frontend When I apply the SolidusStarterFrontend template to the app Then I should see the following frontend assets and directories added to the app: * `vendor/assets/images/spree/frontend` * `vendor/assets/javascripts/spree/frontend/all.js` * `vendor/assets/stylesheets/spree/frontend/all.css` Current behavior ---------------- SolidusStarterFrontend doesn't add the frontend assets and directories to the app's vendor directory. Bug cause --------- Prior to solidusio/solidus#4251, the Solidus Install generator was adding the frontend assets during installation even if the app didn't have `solidus_frontend` in its bundle. With the issue fixed, SolidusStarterFrontend has to be updated to add these frontend assets by itself.
gsmendoza
added a commit
to solidusio/solidus_starter_frontend
that referenced
this pull request
Feb 22, 2022
Goal ---- We want the frontend assets and directories added to a Solidus app because Solidus extensions assume that these files and directories exist. The Solidus extensions typically link these pre-existing files to their assets. Expected behavior ----------------- Given I have a Solidus app without a frontend When I apply the SolidusStarterFrontend template to the app Then I should see the following frontend assets and directories added to the app: * `vendor/assets/images/spree/frontend` * `vendor/assets/javascripts/spree/frontend/all.js` * `vendor/assets/stylesheets/spree/frontend/all.css` Current behavior ---------------- SolidusStarterFrontend doesn't add the frontend assets and directories to the app's vendor directory. Bug cause --------- Prior to solidusio/solidus#4251, the Solidus Install generator was adding the frontend assets during installation even if the app didn't have `solidus_frontend` in its bundle. With the issue fixed, SolidusStarterFrontend has to be updated to add these frontend assets by itself.
gsmendoza
added a commit
to solidusio/solidus_starter_frontend
that referenced
this pull request
Feb 22, 2022
Goal ---- We want the frontend assets and directories added to a Solidus app because Solidus extensions assume that these files and directories exist. The Solidus extensions typically link these pre-existing files to their assets. Expected behavior ----------------- Given I have a Solidus app without a frontend When I apply the SolidusStarterFrontend template to the app Then I should see the following frontend assets and directories added to the app: * `vendor/assets/images/spree/frontend` * `vendor/assets/javascripts/spree/frontend/all.js` * `vendor/assets/stylesheets/spree/frontend/all.css` Current behavior ---------------- SolidusStarterFrontend doesn't add the frontend assets and directories to the app's vendor directory. Bug cause --------- Prior to solidusio/solidus#4251, the Solidus Install generator was adding the frontend assets during installation even if the app didn't have `solidus_frontend` in its bundle. With the issue fixed, SolidusStarterFrontend has to be updated to add these frontend assets by itself. Implementation -------------- This copies as-is the vendored assets of solidus_frontend from https://github.com/solidusio/solidus/tree/master/core/lib/generators/solidus/install/templates/vendor/assets.
gsmendoza
added a commit
to solidusio/solidus_starter_frontend
that referenced
this pull request
Feb 22, 2022
Goal ---- We want the frontend assets and directories added to a Solidus app because Solidus extensions assume that these files and directories exist. The Solidus extensions typically link these pre-existing files to their assets. Expected behavior ----------------- Given I have a Solidus app without a frontend When I apply the SolidusStarterFrontend template to the app Then I should see the following frontend assets and directories added to the app: * `vendor/assets/images/spree/frontend` * `vendor/assets/javascripts/spree/frontend/all.js` * `vendor/assets/stylesheets/spree/frontend/all.css` Current behavior ---------------- SolidusStarterFrontend doesn't add the frontend assets and directories to the app's vendor directory. Bug cause --------- Prior to solidusio/solidus#4251, the Solidus Install generator was adding the frontend assets during installation even if the app didn't have `solidus_frontend` in its bundle. With the issue fixed, SolidusStarterFrontend has to be updated to add these frontend assets by itself. Implementation -------------- This copies as-is the vendored assets of solidus_frontend from https://github.com/solidusio/solidus/tree/master/core/lib/generators/solidus/install/templates/vendor/assets.
gsmendoza
added a commit
to solidusio/solidus_dev_support
that referenced
this pull request
Jun 8, 2022
This fixes the following error when running `bundle exec rake` in
SolidusPaypalCommercePlatform:
```
An error occurred in a `before(:suite)` hook.
Failure/Error: //= link spree/backend/all.js
Sprockets::FileNotFound:
couldn't find file 'spree/backend/all.js'
Checked in these paths:
/home/gsmendoza/repos/solidusio-contrib/solidus_paypal_commerce_platform/spec/dummy/app/assets/config
/home/gsmendoza/repos/solidusio-contrib/solidus_paypal_commerce_platform/spec/dummy/app/assets/images
/home/gsmendoza/repos/solidusio-contrib/solidus_paypal_commerce_platform/spec/dummy/app/assets/stylesheets
/home/gsmendoza/repos/solidusio-contrib/solidus_paypal_commerce_platform/app/assets/javascripts
/home/gsmendoza/repos/solidusio-contrib/solidus_paypal_commerce_platform/app/assets/stylesheets
```
Cause
-----
`extension:specs`, the default rake task of SolidusPaypalCommercePlatform,
executes the rake tasks and Ruby methods below in the given order:
* The `extension` namespace defined in
`SolidusDevSupport::RakeTasks#install_test_app_task`. This namespace invokes
the `extension:test_app` task
(https://github.com/solidusio/solidus_dev_support/blob/e5fec9c039607ab9e97c3f821cc49f9bbec08796/lib/solidus_dev_support/rake_tasks.rb#L47).
* The Solidus `extension:test_app` task
(https://github.com/solidusio/solidus/blob/07c88ddd1603699939ac0343965fa88dc2e1851a/core/lib/spree/testing_support/extension_rake.rb#L7)
* The `common:test_app` rake task in `CommonRakeTasks#initialize`
(https://github.com/solidusio/solidus/blob/07c88ddd1603699939ac0343965fa88dc2e1851a/core/lib/spree/testing_support/common_rake.rb#L14)
* `Solidus::InstallGenerator#setup_assets`
(https://github.com/solidusio/solidus/blob/07c88ddd1603699939ac0343965fa88dc2e1851a/core/lib/generators/solidus/install/install_generator.rb#L81)
The `setup_assets` method would only install the `all.*` frontend and backend
assets if either
* `Spree::Frontend` and `Spree::Backend` are loaded, or
* The Rails environment is test.
Currently, when we run `bundle exec rake` in SolidusPaypalCommercePlatform,
neither condition is met:
* `Spree::Frontend` and `Spree::Backend` are NOT loaded.
* The Rails environment is development.
Trigger
-------
This bug surfaced after we applied
solidusio/solidus#4251 (Fix: solidus:install
adds the frontend assets even if the repo does not have
`solidus_frontend`).
Solution
--------
Considering that the `extension:test_app` rake task generates the
`spec/dummy` directory, we can assume that it is expected to run under
test environment.
2 tasks
gsmendoza
added a commit
to gsmendoza/solidus
that referenced
this pull request
Jun 9, 2022
This fixes the following error when running `bundle exec rake` in
SolidusPaypalCommercePlatform:
```
An error occurred in a `before(:suite)` hook.
Failure/Error: //= link spree/backend/all.js
Sprockets::FileNotFound:
couldn't find file 'spree/backend/all.js'
Checked in these paths:
/home/gsmendoza/repos/solidusio-contrib/solidus_paypal_commerce_platform/spec/dummy/app/assets/config
/home/gsmendoza/repos/solidusio-contrib/solidus_paypal_commerce_platform/spec/dummy/app/assets/images
/home/gsmendoza/repos/solidusio-contrib/solidus_paypal_commerce_platform/spec/dummy/app/assets/stylesheets
/home/gsmendoza/repos/solidusio-contrib/solidus_paypal_commerce_platform/app/assets/javascripts
/home/gsmendoza/repos/solidusio-contrib/solidus_paypal_commerce_platform/app/assets/stylesheets
```
Cause
-----
`bundle exec rake` executes [`Solidus::InstallGenerator#setup_assets`](https://github.com/solidusio/solidus/blob/07c88ddd1603699939ac0343965fa88dc2e1851a/core/lib/generators/solidus/install/install_generator.rb#L81),
which would only install the `all.*` frontend and backend assets if either
* `Spree::Frontend` and `Spree::Backend` are loaded, or
* The Rails environment is test.
Currently, when `bundle exec rake` executes
[`common:test_app`](https://github.com/solidusio/solidus/blob/07c88ddd1603699939ac0343965fa88dc2e1851a/core/lib/spree/testing_support/common_rake.rb)
to build the dummy app, it assigns the `RAILS_ENV` environment variable to
`'test'`. However, if `Rails.env` has already been called, for example, in the
`require ENV['LIB_NAME']` statement before the `RAILS_ENV` assigment, then
`Rails.env` would
[retain its value](https://github.com/rails/rails/blob/3872bc0e54d32e8bf3a6299b0bfe173d94b072fc/railties/lib/rails.rb#L72),
which would most likely be `'development'`.
Trigger
-------
This bug surfaced after we applied
solidusio#4251 (Fix: solidus:install adds the
frontend assets even if the repo does not have `solidus_frontend`).
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Expected behavior
Given I have a Rails app
And the Rails app only has the following Solidus gems:
When I run
bin/rails generate solidus:install --auto-acceptThen I should see that the
vendor/assets/*/spreedirectories do not include frontend directories with its assets.Actual behavior
The
vendor/assets/*/spreedirectories include the frontend directories with their assets.Cause
The
defined? Spree::Frontend || Rails.env.test?in https://github.com/solidusio/solidus/blob/f524772df264fc4fffa971b992d2724f5060c1b7/core/lib/generators/solidus/install/install_generator.rb returns"expression".Fix
Wrapping the argument of the
defined?call in a parenthesis fixes the issue, for example:Similar issues
The backend assets have the same issue and fix.
Tested in
Demo
https://drive.google.com/file/d/1R-rWkHXfTmVOmv32_jV3QliMybFJ2YSP/view?usp=sharing
(Sorry, my lips are not in sync with the audio.)
Checklist: