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

Deprecate solidus_frontend & allow installing solidus_starter_frontend #4490

Conversation

waiting-for-dev
Copy link
Contributor

@waiting-for-dev waiting-for-dev commented Aug 5, 2022

Summary

We're recommending new stores use solidus_starter_frontend as their storefront. However, we still want to give the option to install solidus_frontend for now, as some extensions rely on it. We're modifying the Solidus installer to allow installing one or the other.

Nonetheless, solidus_frontend is part of the solidus meta-gem, which is what is usually bundled in a new installation. At this point, we can't remove solidus_frontend from the meta-gem, as users upgrading from old Solidus versions could see their storefronts suddenly gone. As that will be a breaking change, we're deferring it until v4.0.

Until that happens, we must prevent users choosing the new frontend from pulling solidus_frontend. Because of that, we're breaking down the solidus gem into solidus_core, solidus_api, solidus_backend & solidus_sample. We leverage Bundler's injector to add dependencies matching the main solidus gem constraints in the Gemfile. We also use it to remove the meta-gem. Still, we need to extend the upstream injector to support the path: option, which is required for local testing (e.g., the sandbox application). We also must break down the meta-gem when solidus_frontend is installed until we remove the frontend directory from this repo. That prevents Bundler from resolving it from there when path: or git: are used as sources.

As per the logic to detect which frontend gets installed, the first of the following conditions met wins:

  • It's explicitly selected with the FRONTEND environment variable
    (useful for sandbox applications).
  • It's explicitly selected with the frontend option.
  • If solidus_frontend is in the Gemfile, that's the one used.
  • If the auto_accept option is given, solidus_starter_frontend is
    chosen.
  • The user is prompted to select one or the other.

Extensions have been modified to explicitly depend on solidus_frontend on their Gemfile, so the dummy app generator used for testing will work OOO matching the third condition. As per its sandbox application, they'll default to solidus_starter_frontend because they use auto_accept, but they can select solidus_frontend through the FRONTEND variable.

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.

@waiting-for-dev waiting-for-dev requested a review from a team August 5, 2022 10:59
@waiting-for-dev waiting-for-dev force-pushed the waiting-for-dev/install_solidus_starter_frontend branch from 8dc8993 to f43a087 Compare August 5, 2022 11:12
waiting-for-dev added a commit to solidusio/solidus_starter_frontend that referenced this pull request Aug 8, 2022
We need to reflect that solidus_starter_frontend is now installed by
default by Solidus.

See solidusio/solidus#4490
Copy link
Member

@aldesantis aldesantis left a comment

Choose a reason for hiding this comment

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

This is some top-notch work, thanks!

@gsmendoza
Copy link
Contributor

gsmendoza commented Aug 9, 2022

Hi @waiting-for-dev ! I'm encountering the following error when I try to install SolidusFrontend:

image

Here's the Rails app with SolidusFrontend: https://github.com/gsmendoza/rails_7_store/tree/solidus--solidus-4490--solidus-frontend.

I didn't encounter any issues with the SolidusStarterFrontend version: https://github.com/gsmendoza/rails_7_store/tree/solidus--solidus-4490--solidus-starter-frontend. I was able to pass the specs and complete a checkout.

@gsmendoza
Copy link
Contributor

gsmendoza commented Aug 9, 2022

For comparison, here's the Rails app without any frontend: https://github.com/gsmendoza/rails_7_store/tree/solidus--solidus-4490--no-frontend. No issues encountered so far; I was able to start the server on it.

@waiting-for-dev waiting-for-dev force-pushed the waiting-for-dev/install_solidus_starter_frontend branch from f43a087 to 92c4055 Compare August 9, 2022 03:46
@waiting-for-dev
Copy link
Contributor Author

Hi @waiting-for-dev ! I'm encountering the following error when I try to install SolidusFrontend:

Thanks for checking it out, @gsmendoza!

That's weird. @kennyadsl also found something similar. The issue comes from the fact that solidus_frontend 3.2.0.alpha can come from two sources: the mono-repo & RubyGems (released from the forked repo). Only the latter contains the install generator. In my tests, that was the one selected by Bundler. However, fetching from git: instead of path: showed a warning about conflicting sources, but it picked RubyGems nonetheless. That is a short-lived issue, as there won't be conflicting sources right next when we remove the frontend folder from the mono-repo.

I've updated the code so that solidus_frontend is added to the Gemfile with an explicit source pointing to RubyGems. @gsmendoza, could you give it another try? 🙏

@waiting-for-dev waiting-for-dev force-pushed the waiting-for-dev/install_solidus_starter_frontend branch from 92c4055 to 871b484 Compare August 9, 2022 03:56
@waiting-for-dev
Copy link
Contributor Author

I also rebased from master after #4494

@gsmendoza
Copy link
Contributor

Thanks @waiting-for-dev! I am now able to install Solidus with SolidusFrontend. I used the following Rails app: https://github.com/gsmendoza/rails_7_store/tree/solidus--solidus-4490--solidus-frontend--explicit-source.

We're recommending new stores use solidus_starter_frontend [1] as their
storefront. However, we still want to give the option to install
solidus_frontend for now, as some extensions rely on it. We're modifying
the Solidus installer to allow installing one or the other.

Nonetheless, solidus_frontend is part of the solidus meta-gem [2], which
is what is usually bundled in a new installation. At this point, we
can't remove solidus_frontend from the meta-gem, as users upgrading from
old Solidus versions could see their storefronts suddenly gone. As that
will be a breaking change, we're deferring it until v4.0.

Until that happens, we must prevent users choosing the new frontend from
pulling solidus_frontend. Because of that, we're breaking down the
solidus gem into solidus_core, solidus_api, solidus_backend &
solidus_sample. We leverage Bundler's injector [3] to add dependencies
matching the main solidus gem constraints in the Gemfile. We also use it
to remove the meta-gem. Still, we need to extend the upstream injector
to support the `path:` option, which is required for local testing
(e.g., the sandbox application). We also must break down the meta-gem
when solidus_frontend is installed until we remove the frontend
directory from this repo. That prevents Bundler from resolving it from
there when `path:` or `git:` are used as sources.

As per the logic to detect which frontend gets installed, the first of
the following conditions met wins:

- It's explicitly selected with the `FRONTEND` environment variable
  (useful for sandbox applications).
- It's explicitly selected with the `frontend` option.
- If `solidus_frontend` is in the Gemfile, that's the one used.
- If the `auto_accept` option is given, `solidus_starter_frontend` is
  chosen.
- The user is prompted to select one or the other.

Extensions have been modified to explicitly depend on `solidus_frontend`
on their Gemfile, so the dummy app generator used for testing will work
OOO matching the third condition. As per its sandbox application,
they'll default to `solidus_starter_frontend` because they use
`auto_accept`, but they can select `solidus_frontend` through the
`FRONTEND` variable.

[1] - https://github.com/solidusio/solidus_starter_frontend
[2] - https://github.com/solidusio/solidus/blob/a22723079f88387f53f72b503db875b9d97aa3f5/solidus.gemspec#L27
[3] - https://github.com/rubygems/bundler/blob/master/lib/bundler/injector.rb
@waiting-for-dev
Copy link
Contributor Author

waiting-for-dev commented Aug 9, 2022

I also updated the message prompting to install solidus_auth_devise so that users are aware that it'll be installed anyway if they choose solidus_starter_frontend

gsmendoza pushed a commit to solidusio/solidus_starter_frontend that referenced this pull request Aug 9, 2022
We need to reflect that solidus_starter_frontend is now installed by
default by Solidus.

See solidusio/solidus#4490
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.

I also updated the message prompting to install solidus_auth_devise so that users are aware that it'll be installed anyway if they choose solidus_starter_frontend

It works for now! I think it's acceptable for this transition period.

Thanks @waiting-for-dev, great work as always!

@waiting-for-dev waiting-for-dev merged commit 935c8ae into solidusio:master Aug 9, 2022
@waiting-for-dev waiting-for-dev deleted the waiting-for-dev/install_solidus_starter_frontend branch August 9, 2022 08:58
waiting-for-dev added a commit to nebulab/solidus that referenced this pull request Aug 11, 2022
ebfd2ed changed the installer to allow
users select solidus_starter_frontend or solidus_frontend as their
storefront.

When solidus_frontend was added to the Gemfile, the `source:` argument
was given to avoid conflicts with the `frontend` directory in the
mono-repo. However, the directory was removed in
c89fbf0.

See solidusio#4490 (comment) for more context
waiting-for-dev added a commit to nebulab/solidus that referenced this pull request Aug 15, 2022
ebfd2ed changed the installer to allow
users select solidus_starter_frontend or solidus_frontend as their
storefront.

If the installer added solidus_frontend to the Gemfile, the `source:`
argument was given to avoid conflicts with the `frontend` directory in
the mono-repo. However, the directory was removed in
c89fbf0.

See solidusio#4490 (comment) for more context
gsmendoza pushed a commit to solidusio/solidus_starter_frontend that referenced this pull request Aug 17, 2022
We need to reflect that solidus_starter_frontend is now installed by
default by Solidus.

See solidusio/solidus#4490
gsmendoza pushed a commit to solidusio/solidus_starter_frontend that referenced this pull request Aug 17, 2022
We need to reflect that solidus_starter_frontend is now installed by
default by Solidus.

See solidusio/solidus#4490
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.

5 participants