solidusio / solidus_dev_support

A collection of tools for developing Solidus extensions.
MIT License
21 stars 27 forks source link

Misc: auth-devise, extracted frontend gem, sqlite #188

Closed elia closed 2 years ago

elia commented 2 years ago

Summary

A few minor fixes and updates after the release of v3.2

Checklist

mergify[bot] commented 2 years ago

It looks like this PR is missing a label to determine the type of change it introduces. The maintainer should add one of the following labels:

Additionally, the maintainer may also want to add one of the following:

Once the correct labels have been set, simply remove the needs changelog label label from this PR so I can merge it.

tvdeyen commented 1 year ago

The code is generated by solidus_dev_support not by my extension. I know added solidus_frontend conditionally into the local Gemfile.

That's ok for now, but preferably solidus_dev_support would not mutate the extension Gemfile it it does not depend on frontend..

waiting-for-dev commented 1 year ago

Oh, I see what you mean. You're running it on an existing extension. I think you're right.

tvdeyen commented 1 year ago

Oh, I see what you mean. You're running it on an existing extension. I think you're right.

@waiting-for-dev

Yes, it is an existing extension and I just updated it to support Solidus 3.2.

I have a default Rakefile in my extension:

require "bundler"
Bundler::GemHelper.install_tasks

require "solidus_dev_support/rake_tasks"
SolidusDevSupport::RakeTasks.install

task default: "extension:specs"

and the rake task suddenly started to add solidus_frontend to my local Gemfile

Exactly, that's the intent of having a generator instead of mandating a solution.

I would add that this whole setup is not covering the new frontend in any way and we'll probably need to come back to it once we start testing all extensions (that require it) with the new one.

@elia

Even for new extensions I do not think this is the right approach. Shouldn't we just point to the right gem in the gemspec of the extension instead, if it depends on frontend? Why mutate the Gemfile at all?

elia commented 1 year ago

@tvdeyen no strings attached to that code, given the goal we can find alternative solutions to it:

The goal was to ensure we're grabbing the master version of solidus_frontend when fetching solidus itself from master. Also it was moving from the assumption that solidus will already depend on solidus_frontend, which AFAIK will be true until we reach v4.

Would adding require: false to that line solve the issue you bumped into?

tvdeyen commented 1 year ago

@tvdeyen no strings attached to that code, given the goal we can find alternative solutions to it:

The goal was to ensure we're grabbing the master version of solidus_frontend when fetching solidus itself from master. Also it was moving from the assumption that solidus will already depend on solidus_frontend, which AFAIK will be true until we reach v4.

Would adding require: false to that line solve the issue you bumped into?

@elia

require: false on solidus_frontend?

I do not see why this should fix the issue with that the Rake task from solidus_dev_support adds the line to the Gemfile

What am I missing?

🤔

tvdeyen commented 1 year ago

So, to recap what is happening (not just for me, but for all extensions that update their code to Solidus 3.2 and that uses solidus_dev_support)

bundle exec rake

is calling the default Rake task added by solidus_dev_support

# Rakefile
require "bundler"
Bundler::GemHelper.install_tasks

require "solidus_dev_support/rake_tasks"
SolidusDevSupport::RakeTasks.install

task default: "extension:specs"

That installs a new dummy app into the extensions and runs specs on it. So far, so good. With the change of this PR the rake task that install the dummy app suddenly adds

gem "solidus_frontend", "~> 3.2"

to the Gemfile of the extension.

It was not before this change.

Preferably it would not do that unless the extension is using solidus_frontend, but even then, the extensions author should be advised to change the gemspec instead. Because the extension needs to depend on the correct version of solidus_frontend if needed.

Am I missing something?

elia commented 1 year ago

Oh I see, I'm not sure that this change is the culprit although it looks like it at first analysis. The change in this PR will only be applied to an existing extension if you run bundle exec solidus extension ., which will run the extension generator again and propose changes.

What you're seeing I think is most likely something related to the solidus installer.

Of course this is just an hypothesis, we need to verify 🕵️ @tvdeyen feel free to ping me on slack for a quick pairing session to sort this out 🙌

tvdeyen commented 1 year ago

@elia my guess that this line is the culprit

https://github.com/solidusio/solidus_dev_support/pull/188/files#diff-f344e31271cf7c15d576fc481bc7a9ebee669b8e0093e6f799b986cb9fcbf4aeR94

I assume that the app generator is adding this line to the Gemfile. Should we remove that and advise extensions authors to depend on solidus_frontend in the gemspec instead?

elia commented 1 year ago

@tvdeyen 🤔 I think I'm still failing to see how that is different from before… my assumption being that a solidus install has always depended on solidus_frontend, is there a way I can reproduce the error locally on some extension?