Add SolidusSupport.starter_frontend_available? to check if SolidusStarterFrontend exists

gsmendoza commented 2 years ago

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 https://github.com/solidusio/solidus_support/pull/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

waiting-for-dev commented 2 years ago

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

wintermeyer commented 2 years ago

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 commented 2 years ago

@waiting-for-dev SolidusSupport.starter_frontend_available? would primarily be used to provide support for Starter FE in Solidus extensions. For example, please see https://github.com/gsmendoza/solidus_hello_world/commit/1eb331620ecb0ca9e7e33901ba9676cf1b674486. 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:

https://github.com/gsmendoza/solidus_legacy_frontend_store/tree/gsmendoza/eng-312-create-a-solidus-extension-for-testing - Solidus app using legacy SolidusFrontend gem
https://github.com/gsmendoza/solidus_frontend_less_store/tree/gsmendoza/eng-312-create-a-solidus-extension-for-testing - Solidus app without any frontend
https://github.com/gsmendoza/solidus_starter_frontend_store/tree/gsmendoza/eng-312-create-a-solidus-extension-for-testing - Solidus app using the Starter Frontend

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

waiting-for-dev commented 2 years ago

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 commented 2 years ago

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

gsmendoza commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

I like your argumentation.

gsmendoza commented 2 years ago

I'm good with not implementing anything. No code = best code :D

On Fri, Mar 11, 2022, 1:27 PM Stefan Wintermeyer @.***> wrote:

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

I like your argumentation.

— Reply to this email directly, view it on GitHub https://github.com/solidusio/solidus_support/pull/69#issuecomment-1064784961, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAPAJDV3WXHIKG4OLJFZ3DU7LKS3ANCNFSM5QIE4AEQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you were mentioned.Message ID: @.***>

solidusio / solidus_support