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

Fork solidus_frontend into a separate repo #1

Merged

Conversation

gsmendoza
Copy link
Contributor

@gsmendoza gsmendoza commented Apr 4, 2022

Dependencies

Dependent on #2.

Status

Wasn't able to update yet:

  • Gem scripts
  • GitHub config

TODOs

  • Test on CircleCI - realized only now that I can probably test the repo on my gsmendoza CircleCI account.

  • Rebuild SolidusLegacyFrontend as a Solidus extension.

  • Instead of pointing SolidusLegacyFrontend to Solidus, point the gem to Solidus' subprojects instead (excluding SolidusFrontend, of course).

  • No longer applicable - Test if renaming the manifest.js will break production apps depending on SolidusFrontend.

  • Update Gemfile - Remove :frontend group.

  • Update README

    • Add notice to let new users know that this is not the recommended frontend anymore.
    • Rename "Testing" to "Contributing".
  • Update Gem summary - We could also link to the new recommended starter frontend.

  • Test if solidus frontend can be added to an app AFTER Solidus has been installed on it (without any frontend).

  • Ensure that SolidusFrontend will allow backward compatibility for Solidus starting with 3.2.0.alpha.

Files

  • ./.circleci

  • ./.dockerdev

    • ./.dockerdev/.psqlrc
    • ./.dockerdev/Dockerfile
      • Got docker-compose build to run.
  • ./.eslintrc.json

  • ./.gem_release.yml

    • bundle exec gem bump
    • bundle exec gem release
  • ./.github

    • ./.github/stale.yml
  • ./.gitignore

  • ./.rubocop.yml

  • ./app

    • Copied from frontend directory.
  • ./bin/console

  • ./bin/rails

  • ./bin/rake

    • bin/rake changelog
    • bin/rake clean
    • bin/rake clobber
    • bin/rake console
    • bin/rake db:create
    • bin/rake db:drop
    • bin/rake db:migrate
    • bin/rake db:reset
    • bin/rake extension:specs
    • bin/rake extension:test_app
    • bin/rake sandbox
  • ./bin/sandbox

  • ./bin/setup

  • ./CHANGELOG.md

  • ./config

    • Copied from frontend directory.
  • ./Gemfile

  • ./Gemfile.lock

  • ./lib

    • Copied from frontend directory.
  • ./LICENSE

  • ./Rakefile

  • ./README.md

  • ./solidus_frontend.gemspec

  • ./spec

    • Copied from frontend directory.

@gsmendoza gsmendoza self-assigned this Apr 4, 2022
@gsmendoza gsmendoza force-pushed the gsmendoza/eng-310-fork-solidus_frontend-into-a-separate branch 3 times, most recently from 92a556f to 0da4946 Compare April 5, 2022 09:12
@gsmendoza gsmendoza changed the base branch from master to gsmendoza/eng-310-fork-solidus_frontend-into-a-separate--inline-frontend April 5, 2022 09:41
Gemfile Outdated
Comment on lines 5 to 6
gem 'solidus', github: 'gsmendoza/solidus',
branch: 'gsmendoza/eng-311-remove-solidus_frontend-from-solidus-gem'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This GitHub branch is temporary.

bin/sandbox Outdated
@@ -53,7 +53,8 @@ fi
cd ./sandbox
cat <<RUBY >> Gemfile

gem 'solidus', path: '..'
gem 'solidus', github: 'gsmendoza/solidus', branch: 'gsmendoza/eng-311-remove-solidus_frontend-from-solidus-gem'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This GitHub branch is temporary.

@gsmendoza gsmendoza force-pushed the gsmendoza/eng-310-fork-solidus_frontend-into-a-separate branch from 0da4946 to 6201cc5 Compare April 5, 2022 09:48
@gsmendoza gsmendoza force-pushed the gsmendoza/eng-310-fork-solidus_frontend-into-a-separate--inline-frontend branch from c1007e6 to fce3ee9 Compare April 5, 2022 10:15
@gsmendoza gsmendoza force-pushed the gsmendoza/eng-310-fork-solidus_frontend-into-a-separate branch from 6201cc5 to f73eb42 Compare April 5, 2022 10:15
Gemfile Outdated
@@ -48,7 +51,7 @@ group :backend, :frontend, :core, :api do
gem 'factory_bot_rails', '>= 4.8', require: false
end

group :backend, :frontend do
group :frontend do
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably we can remove wrapping with group :frontend altogether. I mean, now, that's THE group 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the groups in the new update.

Rakefile Outdated
@@ -1,5 +1,6 @@
# frozen_string_literal: true

require 'bundler'
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably, for the Rakefile and CI, we should take now what extensions do. Take solidus_subscriptions as an example. Do you think that's something doable here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like the idea of rebuilding SolidusLegacyFrontend as a Solidus extension. It would make the support code consistent with the extensions we already have. I'll give it a try 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I rebuilt SolidusFrontend as an extension in the new PR update.

bin/build Outdated
@@ -7,28 +7,7 @@ export DB=${DB:-postgresql}

bin/setup

echo "***********************"
Copy link
Contributor

Choose a reason for hiding this comment

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

Similarly, could we use the extensions format for the bin folder?

@@ -1,3 +1,3 @@
# frozen_string_literal: true

Rails.application.config.assets.precompile << 'solidus_frontend_manifest.js'
Rails.application.config.assets.precompile << 'solidus_legacy_frontend_manifest.js'
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't it going to break applications using solidus_frontend now? If so, we could just keep the old name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if it will break apps in productions. I'll test it first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I retained the SolidusFrontend name in the new PR update.

s.name = 'solidus_frontend'
s.version = Spree.solidus_version
s.name = 'solidus_legacy_frontend'
s.version = Spree::Frontend.version
s.summary = 'Cart and storefront for the Solidus e-commerce project.'
Copy link
Contributor

Choose a reason for hiding this comment

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

We could also link to the new recommended starter frontend.

Copy link
Contributor

@waiting-for-dev waiting-for-dev left a comment

Choose a reason for hiding this comment

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

Thanks, @gsmendoza for this master-watchmaker job 🙂

Other comments:

  • I would add a notice in the README to let new users know that this is not the recommended frontend anymore.
  • We're not adding tests to the host application if I'm not wrong. So I think the "Testig" section in the README should instead be called "Contributing".
  • Did you try running tests on docker?

@gsmendoza
Copy link
Contributor Author

Thank you @waiting-for-dev for the review and the kind compliment :) I'm listing my TODOs for this below. Let me know I missed anything:

  • Test on CircleCI - realized only now that I can probably test the repo on my gsmendoza CircleCI account.

  • Rebuild SolidusLegacyFrontend as a Solidus extension.

  • Instead of pointing SolidusLegacyFrontend to Solidus, point the gem to Solidus' subprojects instead (excluding SolidusFrontend, of course).

  • Test if renaming the manifest.js will break production apps depending on SolidusFrontend.

  • Update Gemfile - Remove :frontend group.

  • Update README

    • Add notice to let new users know that this is not the recommended frontend anymore.
    • Rename "Testing" to "Contributing".
  • Update Gem summary - We could also link to the new recommended starter frontend.

@waiting-for-dev
Copy link
Contributor

Instead of pointing SolidusLegacyFrontend to Solidus, point the gem to Solidus' subprojects instead (excluding SolidusFrontend, of course).

I think you'll only need solidus_core & solidus_api.

Looks good @gsmendoza, thanks!!

@gsmendoza gsmendoza changed the base branch from gsmendoza/eng-310-fork-solidus_frontend-into-a-separate--inline-frontend to master April 11, 2022 07:08
@gsmendoza gsmendoza force-pushed the gsmendoza/eng-310-fork-solidus_frontend-into-a-separate branch 5 times, most recently from 344a2d6 to 7eba458 Compare April 12, 2022 10:33
@gsmendoza gsmendoza force-pushed the gsmendoza/eng-310-fork-solidus_frontend-into-a-separate branch 5 times, most recently from eef7a8e to 5a32917 Compare April 26, 2022 10:07
Ensured that `bundle exec rake changelog` is working.
Fixes following error when running `rake extension:specs`:

```
Don't know how to build task 'db:reset'
```
Ensure that sandbox loads only the SolidusFrontend from the root dir.
Also add a warning that since the frontend engine is still part of the
Solidus meta-gem, we need to make sure to enable these configurations
only if they haven't already been enabled in the `spree.rb` initializer.
Code doesn't pass RuboCop.
When SolidusFrontend was part of Solidus, its version was lockstep
with the other components of Solidus.  As such, during this fork,
there's no need to make SolidusFrontend 3.2.0.alpha compatible with
older versions of Solidus. However, future versions of SolidusFrontend
would be expected to provide backward compatibility to supported
versions of Solidus.
Fixes `Webdrivers::BrowserNotFound` errors in CI.
* Add notice to let new users know that this is not the recommended
  frontend anymore.

* Rename "Testing" to "Contributing".
Remove bundler workaround. No longer needed.
* Update GitHub and CircleCI links.
* Change `bin/rspec` to `bundle exec rspec`.
* Remove section on running Solidus test suites.
@gsmendoza gsmendoza force-pushed the gsmendoza/eng-310-fork-solidus_frontend-into-a-separate branch from 2feeea4 to b4ea5c6 Compare July 4, 2022 09:29
@gsmendoza
Copy link
Contributor Author

Hi @kennyadsl . I made the following updates. Appreciate if you can review!

  • Fix: Remove some non-frontend code from Solidus that I missed in Inline Solidus Frontend #2 .
  • Fix: Decouple the Solidus API and Core versions from the frontend version
  • Fix: Ensure that SolidusFrontend can be installed on an app that already had Solidus installed on it (without a frontend).

Walkthrough

2022-07-04.17-10-47-solidus-frontend-pr-1-update.mp4

Demo

2022-07-04.17-41-00.solidus-frontend.pr.1.demo.mp4

Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

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

👍 Thanks George!

@kennyadsl kennyadsl merged commit c91cdee into master Jul 4, 2022
@kennyadsl kennyadsl deleted the gsmendoza/eng-310-fork-solidus_frontend-into-a-separate branch July 4, 2022 15:15
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.

None yet

3 participants