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

Add SolidusSupport.starter_frontend_available? to check if SolidusStarterFrontend exists #69

Closed
wants to merge 1 commit into from

Conversation

gsmendoza
Copy link
Contributor

Where does the Solidus ecosystem currently use SolidusSupport.frontend_available?

  • The extension's engine checks if frontend is available before adding
    assets to the app's lib and precompile paths.

  • The EngineExtensions module from SolidusSupport automatically adds the
    extension's frontend classes to the app's lib.

Why not just update SolidusSupport.frontend_available? to check if Starter FE is available?

While updating frontend_available? doesn't seem to break an app that uses the
legacy SolidusFrontend gem, it currently doesn't work with
SolidusStarterFrontend. This is because the updated frontend_available?
allows SolidusAuthDevise to load its (legacy) frontend code to a Starter
FE-based app. SolidusStarterFrontend already generates its own authentication
frontend code, so the legacy frontend code that is loaded by SolidusAuthDevise
would conflict with the generated Starter FE authentication code.

It seems better to keep the definition of frontend_available? unchanged, that
is, for the method to refer to the legacy frontend gem.

See #68 for a previous attempt to change SolidusSupport.frontend_available? to check if Starter FE is available.

Requirement

Given I have SolidusStarterFrontend installed on my Solidus app
When I call SolidusSupport.starter_frontend_available?
Then it should return true

…rterFrontend exists

Where does the Solidus ecosystem currently use `SolidusSupport.frontend_available?`
-----------------------------------------------------------------------------------

* The extension's engine checks if frontend is available before adding
  assets to the app's lib and precompile paths.

* The EngineExtensions module from SolidusSupport automatically adds the
  extension's frontend classes to the app's lib.

Why not just update `SolidusSupport.frontend_available?` to check if Starter FE is available?
---------------------------------------------------------------------------------------------

While updating `frontend_available?` doesn't seem to break an app that uses the
legacy SolidusFrontend gem, it currently doesn't work with
SolidusStarterFrontend. This is because the updated `frontend_available?`
allows SolidusAuthDevise to load its (legacy) frontend code to a Starter
FE-based app. SolidusStarterFrontend already generates its own authentication
frontend code, so the legacy frontend code that is loaded by SolidusAuthDevise
would conflict with the generated Starter FE authentication code.

It seems better to keep the definition of `frontend_available?` unchanged, that
is, for the method to refer to the legacy frontend gem.

Requirement
-----------

Given I have SolidusStarterFrontend installed on my Solidus app
When I call `SolidusSupport.starter_frontend_available?`
Then it should return true
@waiting-for-dev waiting-for-dev self-requested a review March 9, 2022 06:06
@waiting-for-dev
Copy link
Contributor

Hey @gsmendoza, can you help me understand the big picture? Where are you going to call #starter_frontend_available? from?

@wintermeyer
Copy link

wintermeyer commented Mar 9, 2022

I am not a big fan of having starter_frontend_available? and frontend_available? as parallel method names. I feel that it would make more sense to have frontend_available? and make starter_frontend a parameter for this method. Add a default value for that parameter for the old use.

But I understand @gsmendoza argument. I still feel that we should/must find a clean(er) solution for this.

@gsmendoza
Copy link
Contributor Author

@waiting-for-dev SolidusSupport.starter_frontend_available? would primarily be used to provide support for Starter FE in Solidus extensions. For example, please see gsmendoza/solidus_hello_world@1eb3316. In this commit, I updated the install generator of a dummy "Hello World" Solidus extension to generate the frontend files for apps that use SolidusStarterFrontend.

Here are some Solidus apps where I tested that SolidusHelloWorld extension install generator:

@wintermeyer I'm okay with with the starter_frontend parameter idea. @waiting-for-dev What are your thoughts about it?

@waiting-for-dev
Copy link
Contributor

For the method vs. parameter discussion, I'd go with different methods. As the caller already knows which path it wants to take, I think it'd be a case of control coupling.

Anyway, I agree that asking from within the extension is not ideal, as we're simulating polymorphism. Maybe we could have the core components providing hook points or something else. But I'm afraid that would require changing A LOT of things...

@wintermeyer
Copy link

hmmm... what do we do if there is the need for another abc and xyz frontend? How would we name the _available? methods?

@gsmendoza
Copy link
Contributor Author

@wintermeyer I like what @waiting-for-dev said about control coupling. I think the best time to switch to a parameter-based solution is when the choice of frontend has be decided during runtime.

hmmm... what do we do if there is the need for another abc and xyz frontend? How would we name the _available? methods?

Our current direction is to replace the SolidusFrontend gem with the Starter FE. It's unlikely that we'll add new frontends in the future. However, even if we do, I think what I said earlier still applies: the best time to switch to a parameter-based solution is when the choice of frontend will be decided during runtime. It might be clunky having a several methods for the frontends, but with only a few of them, it's probably tolerable hehe :)

@wintermeyer
Copy link

Should we rename frontend_available? because it is misleading for anybody who looks the first time into the code and doesn't know the history. As a newbie it would make more sense to me to have something like legacy_frontend_available?

@waiting-for-dev
Copy link
Contributor

After thinking about it, I think the best solution here would be to do nothing! Let me explain better.

As the edge guides point out (and @gsmendoza shared in another channel), we want to discourage solidus extensions from providing storefront code. The reasons for that are that many different storefronts exist, plus they're usually customized so that it's difficult that an extension can integrate seamlessly.

Because of that, we don't want to provide an officially endorsed method to do that. If an extension really wants to extend one of the specific frontends, they can check themselves for the existence of, for instance, the Solidus::StarterFrontend constant.

Before reaching that conclusion, I had thought about coming up with a solution similar to the one proposed in #66. That's to say:

  • Having Solidus::Frontend register a Rails.configuration.x.solidus.frontend = 'legacy_frontend' setting.
  • Having Solidus::StarterFrontend register a Rails.configuration.x.solidus.frontend = 'starter_frontend' setting.
  • Implementing a SolidusSupport .solidus_frontend_identifier method, which would return the value of Rails.configuration.x.solidus.frontend.
  • Let the extension get the identifier and decide the path to take.

The above would allow having the system open for extension (create a new system) but closed for modification (that wouldn't require updating SolidusSupport).

However, as I said before, probably the best thing is to do nothing about it, as that's a behavior we want to discourage.

Thoughts?

@wintermeyer
Copy link

After thinking about it, I think the best solution here would be to do nothing!

I like your argumentation.

@gsmendoza
Copy link
Contributor Author

gsmendoza commented Mar 11, 2022 via email

@gsmendoza gsmendoza closed this Mar 14, 2022
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.

3 participants