search

solidusio / solidus_support

Common runtime helpers for Solidus extensions.
BSD 3-Clause "New" or "Revised" License
9 stars 23 forks source link

Support Rails 5.1.x #51

Closed brchristian closed 3 years ago

brchristian commented 4 years ago

All of the currently released Solidus versions still support Rails 5.1.x, and I think it's important to try to continue to support it as long as possible in this extension in particular.

Many Solidus extensions rely on solidus_support v 0.5 and greater, and at the moment our store cannot get any of the latest commits from solidus_tax_cloud or solidus_handling_fees or solidus_stripe or solidus_related_products or . . . you get the idea . . . until we transition our application to Rails 5.2. (This is a huge undertaking, because it means somehow getting away from the deprecated solidus_active_shipping!)

Because this extension is so widely used (indeed that's the whole idea!), I believe it is important to have compatibility requirements that are as loose as possible, otherwise any requirements here will constrain the entire ecosystem of a user's Solidus extensions.

I'm not familiar with what if anything changed in activesupport from v5.1 to 5.2, but all of the specs are green and, as I say, all current versions of Solidus support 5.1 so I think we should endeavor to continue to do so until there is a convincing reason otherwise!

aldesantis commented 4 years ago

@brchristian thanks for the PR! You're raising some valid concerns here.

I was thinking it may be an even better idea to remove the version constraints on activesupport entirely. Instead, let's rely on solidus_core etc. specifying the required Rails version.

What do you think?

brchristian commented 4 years ago

@aldesantis I support this idea! I think that makes a lot of sense. And of course individual extensions can have constraints on the Rails version, too, if they make use of version-specific Rails features.

brchristian commented 4 years ago

Hi @aldesantis I’ve gone ahead and done exactly as you suggested, removing the constraint on activesupport entirely. What do you think?

brchristian commented 4 years ago

@jarednorman great idea, added it!

brchristian commented 3 years ago

@aldesantis @jarednorman Anything I can do to help this along? We’ve been running this in our own fork in production for a few months now and would love to rejoin the party on the master branch!

aldesantis commented 3 years ago

@brchristian sorry, somehow I missed the notification for this one! LGTM, @jarednorman what do you think?

brchristian commented 3 years ago

Terrific, thanks all!