Relax activesupport dependency #236

alessandro-fazzi · 2022-06-27T14:30:48Z

Given #226 and the bleeding announced in #153 (comment) the goal of this PR is to relax the dependency this gem has on ActiveSupport.

Why not removing it completely?

It turns that the only part of the gem which continues to require ActiveSupport are the Rails generators. They were introduced recently in #194. Thus I cannot afford to remove ActiveSupport with a one-shot PR. Nor I want to take decisions about how to continue from now on w/o including maintainer and community into the discussion.

Is it worth to add more dependencies w/o removing ActiveSupport?

Ideally not, but from a transitional perspective, yes. Now we have a single well scoped feature requiring ActiveSupport instead of having code bites spread all over the code base.
I'm bringing the gem in a more free and easier to manage position in regard of AS dep.

What about the two new dependencies?

I added https://dry-rb.org/gems/dry-inflector/ and https://github.com/schmidt/structured_warnings

Both were choosen because they are small, dependency-free gems with single functionality.

dry-inflector is from the (beloved by me) dry-rb ecosystem (https://dry-rb.org/). Neat, iper-atomic and well written. It is the Hanami inflector.

structured_warnings is a neat, very small gem; it has not been updated recently, but IMO it doesn't need to be. It works on ruby 3.1 and its approach is simple and far (FAR) less polluting than ActiveSupport's one. I was able to implement a custom matcher for rspec too, writing expressive tests. Moreover its interface made possible to do a surgical substitution in the actual code base.

From my point of view, being the maintainer of the project, I'd think about splitting rails generators into a separate gem. This would enclose ActiveSupport and Rails-only things in cleaner context, easier to maintain. And the test suite of the "core" gem would lose all the complexity inherent to testing against all the possible AS versions.

I thought to open this one as a draft, but I decided to concretely propose something :) No problem turning it into draft or to use this just as a discussion desk and then close it.

Thanks for your attention.

We're substituting it with dry-inflector, which is a zero deps, stand-alone, imperative inflector. We're also explicitly requiring `i18n` dependency in order to not relay on the indirect dependency through `activesupport`.

adomokos · 2022-06-28T18:40:57Z

This is a great PR! I thank you for the work you put into it, I am going to merge it.
If I understand this correctly, after your change, the only dependency we have for AS is the Rails generator.

You wrote:

From my point of view, being the maintainer of the project, I'd think about splitting rails generators into a separate gem

I am all for doing this, but we should move LightService under an org, and have the gem, and this other one with Rails generator should be there. What do you think?

adomokos · 2022-06-28T18:50:40Z

light-service.gemspec

@@ -18,6 +18,9 @@ Gem::Specification.new do |gem|
  gem.required_ruby_version = '>= 2.6.0'

  gem.add_runtime_dependency("activesupport", ">= 4.0.0")
+  gem.add_runtime_dependency("i18n")
+  gem.add_runtime_dependency("dry-inflector")
+  gem.add_runtime_dependency("structured_warnings")


Oh, I missed these additions. So instead of having just AS, we are adding 3 more dependencies.
I wish we could achieve the same functionality with no additional gems. But maybe I am just asking too much.

I wonder what ppl think about this change.

We’re adding only 2 more dependencies, since i18n is already in through AS at the moment.

I explicitly required it as a dep in order to have a functioning gemspec even considering AS removal and without leaving remainders for future work.

Speaking about the balance between AS and the two new gems: my sentiment is that with the two gems we bring less code than with AS alone. We should analyze it more scientifically tho.

For sure we have the achievement of not having a big rails ecosystem’s gem, leaving LS more desirable as cross-framework and franework-less solution.

Speaking of deprecation warnings: I previously tried to implement them using Gem::Deprecate, but it resulted to me as an inadequate tool out of its absolute standard concept. Hard to adapt. I think it would be feasible to write a dedicated Logger and implement some logic inside it in order to achieve a simple custom deprecation warner. This would be intended as a pragmatic and simpler alternative to structured_warning gem.
If you’d like to check a draft implementation I could try to write a separate draft PR in the second half of July.

About the inflector I’m really opinionated: I would not even try to reinvent the wheel on such a matter. Moreover I sincerely invite you to read dry-inflector’s source code: it’s astonishing small, tidy and simple; and it’s a completely standalone gem. Obviously we could copy that few methods we need, but pro and cons considered I think it’s a solid choice to bring it in.

Thanks for the feedback 😊

adomokos · 2022-06-28T18:55:15Z

@gee-forr - what do you think?

alessandro-fazzi · 2022-06-28T20:57:37Z

This is a great PR! I thank you for the work you put into it, I am going to merge it. If I understand this correctly, after your change, the only dependency we have for AS is the Rails generator.

You wrote:

From my point of view, being the maintainer of the project, I'd think about splitting rails generators into a separate gem

I am all for doing this, but we should move LightService under an org, and have the gem, and this other one with Rails generator should be there. What do you think?

Reading again the code, I’d like to do a couple of little refinements. But I’ll let the discussion go further before to do anything else.

I think, consolidated the choice to split the code, the organization would be the more elegant solution and the better guarantee for the community.
But pragmatically speaking it wouldn’t be mandatory.

I don’t think an organization would be that big management complication, but it’s a personal point of view and a matter to think about before going all-in.

gee-forr · 2022-06-29T10:18:55Z

@adomokos - I really appreciate you asking for my opinion ❤️

@pioneerskies - Thank you so much for the PR and the interest.

I haven't looked at the actual changes yet, but here are my opinions, off the cuff.

We cannot remove rails generators without marking them for deprecation, and then removing them in a major release. So, if we release a 0.19.0 and they output deprecation warnings whenever rails generators are used, that's perfect.
Moving LS to an org would be nice - it'll also make discovering gems in the LS ecosystem easier. I suspect @douglasgreyling - author of light-service.js and light-service.php would be keen to move his projects over to the org too.
Moving rails generators to a light-service-rails gem would be great - assuming deprecation is handled responsibly, and would make maintaining the two gems simpler. I would want to match versions between them so that there's no confusion between which version of light service the rails gem bundles in. As it is, there are some bugs/quirks with the generators that I'm not happy with that I've been meaning to fix, but don't really want to impose a version bump on LS as a result.

gee-forr

Looking good... one note about the re-requiring of structured_warnings but everything else looks good.

gee-forr · 2022-06-29T10:21:07Z

lib/light-service.rb

@@ -1,5 +1,6 @@
 require 'logger'
-require 'active_support/core_ext/string'
+require 'i18n'
+require 'structured_warnings'


If we're requiring structured_warnings in light-service.rb we probably don't need to re-require them again in the rest of the files, right?

Totaly agree: OR in the manifest OR in single files. In both places is just confusing and conceptually wrong.

gee-forr · 2022-06-29T10:24:20Z

spec/acceptance/include_warning_spec.rb

+      expect do
+        class OrganizerIncludingLS
+          include LightService::Organizer
+        end
+      end.to warn_with(StructuredWarnings::DeprecatedMethodWarning, expected_msg)


These tests are great! We should remember to remove them when the actual removal takes place. 👍🏻

Given this discussion #236 (comment) it's still to take a decision about the use of that library.

That said and considering that whatever implementation will be it will have a test: remembering to remove deprecation from code will bring a red CI. That should be a good post-it to remove the test too ;)

gee-forr · 2022-06-29T10:27:10Z

@adomokos - if you're happy to move forward with this, I am. I think adding two more dependencies is a small and temporary price to pay for what it unlocks for the future.

If you want to move forward with the org + separate rails gem, then, @pioneerskies could I ask that you also add deprecation warnings to the rails generators?

alessandro-fazzi · 2022-06-29T12:36:27Z

If you want to move forward with the org + separate rails gem, then, @pioneerskies could I ask that you also add deprecation warnings to the rails generators?

I'd be happy to do, once decisions will be made, but I'd opt to do it in a dedicated PR: current PR opens the question, but it doesn't want to directly move away generators. A new PR using this one as backbone would be clearer and tidier IMO.

alessandro-fazzi · 2022-06-29T12:41:56Z

lib/light-service/action.rb

@@ -9,7 +9,7 @@ def self.extended(base_class)
    def self.included(base_class)
      msg = "including LightService::Action is deprecated. " \
            "Please use `extend LightService::Action` instead"
-      ActiveSupport::Deprecation.warn(msg)
+      warn(StructuredWarnings::DeprecatedMethodWarning, msg)


Here and wherever we thrown a warn, if this gem will remain in, I'd like to have a class LightService::Deprecation < StructuredWarnings::Base; end to use insted of directly expose the gem's built-in.

I'm completely for that course of action.

adomokos · 2022-06-29T16:33:15Z

I am afk, but will pick this up, thoroughly review it, and merge it next week. Thanks for your patience there!

alessandro-fazzi · 2022-06-29T17:17:57Z

I am afk, but will pick this up, thoroughly review it, and merge it next week. Thanks for your patience there!

I'll be on vacation starting from 1st until 19th July. In the mean time feel free to do what it's needed with "my" code. I've already annoted and subscribed desirable updates and expressed all the opinions I had to. So feel free to act w/o my intervention in the coming weeks :)

Thank you all so much.

adomokos · 2022-06-29T19:05:01Z

I like this, will merge this. It's not fixing a critical bug. We'll get this in.

adomokos · 2022-08-09T14:38:36Z

Hello @pioneerskies! Sorry for keeping this PR open for so long.

I thought about this while I was away. How hard would it be not to introduce two (albeit small) gems to light-service, but leverage pure Ruby to accomplish this functionality? I'd love to have LS be outside gem-dependent-free if possible in the future.

alessandro-fazzi · 2022-08-11T09:00:55Z

Hello @pioneerskies! Sorry for keeping this PR open for so long.

I thought about this while I was away. How hard would it be not to introduce two (albeit small) gems to light-service, but leverage pure Ruby to accomplish this functionality? I'd love to have LS be outside gem-dependent-free if possible in the future.

Hi there,

AFAIK deprecations could be simplified down to a custom Logger: if my assumption would be right, then writing 1st party code would be easy. I mean "easy" as "not complex", not as "fast", but that's it.

The inflector instead would be different: dry-inflector is the smallest, most low-complexity thing I've found around. Given we don't want to use any 3rd party dep, then I'd opt for brutally copy/paste-ing the gem's lib/ into our codebase. That's because I won't be able to design something different and more simple than that code and reinventing the wheel would be wasted time with a worse final result (since I'm honestly light years far from Luca's and Peter's design/coding skills :) )

Conclusion: a custom "deprecation logger" + a savvy copy-paste should result in a quite small and not complex goal to achieve.

adomokos · 2022-08-11T13:24:57Z

I see adding 3 ruby gems with this PR:

i18n - which needs concurrent-ruby
dry-inflector - no external library dependency
structured_warnings - no external library dependency

I do like the intent of this PR, but I am now hesitant to merge it. We are adding quite a bit of dependency, and I don't see the path of cutting out activesupport altogether. I wonder if we should keep this PR open, create a new gem light-service-rails, extract the Rails generator dependencies into that, and then merge this PR. That way we have a guaranteed path of reducing the external gem dependency.

What do you think about that?

I am happy to create a new repo for light-service-rails, I am not sure if I'd create a new organization for it yet. I'd be happy to add you as maintainer to this new project if you're willing to help me extract the Rails and AS dependencies. What do you think?

alessandro-fazzi · 2022-08-12T08:39:53Z

I like to re-underline that i18n here is not added, but just made explicit: it's already an AS's dependency. As it stands it's just a peer dependence through AS but it is explicitly used for failure message translation. Having it as a direct dependence was a way to underline the fact that even in the will to remove AS we'll have to retain i18n.

IMO we should be able to avoid i18n dependency too, making it opt-in: if an implementor would like to use translated messages she should add i18n to her bundle and LS would pick it up automatically. If absent and a Symbol is passed as failure message, LS could drop a warning ad just .to_s the symbol.

I know this i18n-related discourse may seem OT, but I consider part of the "dependent-free" path you are picturing.

Coming on the spot of your comment now :)

I do like the intent of this PR, but I am now hesitant to merge it

I'm not only okay with that, but I'm really happy with that. My intention was to open a discussion bringing a practical substrate to it.

We are adding quite a bit of dependency

I think we're trading dependencies here, more than adding. The biggest fault of this PR, in my opinion, is that it moves from one dep from another w/o layering them with first party wrappers. Wrappers would be a refactoring pattern making possible to plug in/out external deps or first party code w/o touching other parts of code. E.g.:

# lib/light-service/inflector.rb

require "dry/inflector"

module LightService
  class Inflector
    attr_reader :inflector

    def initialize
      @inflector = Dry::Inflector.new
    end

    def unerscore(string)
      inflector.underscore(string)
    end

    def pluralize(string)
      inflector.underscore(string)
    end
  end
end

# lib/light-service/localization_adapter.rb

module LightService
  class LocalizationAdapter
    attr_reader :inflector

    def initialize
      @inflector = LightService::Inflector
    end

    # [...]

    def i18n_scope_from_class(action_class, type)
      "#{inflector.underscore(action_class.name)}.light_service.#{inflector.pluralize(type.to_s)}"
    end
  end
end

This way we'd be able to swap in/out different external deps or get rid of them and write our own or anything we'd like to, with a single point of responsibility

I wonder if we should keep this PR open, create a new gem light-service-rails, extract the Rails generator dependencies into that, and then merge this PR

That path would be for sure the more profitable one.

Current PR had just the goal to make things move forward given the actual code base and to poll the interest around the proposed mid-term goal.

Keep this one open as draft and we'll understand if it will be better to update it or to close it for a new one.

I am happy to create a new repo for light-service-rails, I am not sure if I'd create a new organization for it yet. I'd be happy to add you as maintainer to this new project if you're willing to help me extract the Rails and AS dependencies. What do you think?

don't bother to go for the org atm: let's do some work and then you'll think about organizational matters
I'll be happy to help until the end of September; then my baby-girl will come to the world and I'm ready to bet I'll need to take a pause from free-time work for a while :)
let's check for @gee-forr availability to help too; even just as reviewer. As original author of generator's code and as active user of the feature he may be a great booster

adomokos · 2022-08-12T14:12:27Z

If and when we go with the route of a separate gem version for light-service-rails, we will need to bump the major version to 1.

adomokos · 2024-09-08T16:56:13Z

Closing this, as AS was removed with this PR.

alessandro-fazzi added 2 commits June 27, 2022 12:46

Remove active_support/core_ext/string dependency

60e758c

We're substituting it with dry-inflector, which is a zero deps, stand-alone, imperative inflector. We're also explicitly requiring `i18n` dependency in order to not relay on the indirect dependency through `activesupport`.

Remove active_support/deprecation dependency

047ca3b

adomokos reviewed Jun 28, 2022

View reviewed changes

gee-forr reviewed Jun 29, 2022

View reviewed changes

alessandro-fazzi commented Jun 29, 2022

View reviewed changes

This was referenced Dec 5, 2022

Implement a built-in, dependency free localization adapter #238

Merged

Remove i18n localization adapter #239

Closed

Swap ActiveSupport::Deprecation with built-in LightSupport::Deprecation #241

Merged

adomokos closed this Sep 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Relax activesupport dependency #236

Relax activesupport dependency #236

alessandro-fazzi commented Jun 27, 2022

adomokos commented Jun 28, 2022

adomokos Jun 28, 2022

alessandro-fazzi Jun 28, 2022 •

edited

Loading

adomokos commented Jun 28, 2022

alessandro-fazzi commented Jun 28, 2022

gee-forr commented Jun 29, 2022

gee-forr left a comment

gee-forr Jun 29, 2022

alessandro-fazzi Jun 29, 2022

gee-forr Jun 29, 2022

alessandro-fazzi Jun 29, 2022

gee-forr commented Jun 29, 2022

alessandro-fazzi commented Jun 29, 2022

alessandro-fazzi Jun 29, 2022

gee-forr Jun 29, 2022

adomokos commented Jun 29, 2022

alessandro-fazzi commented Jun 29, 2022

adomokos commented Jun 29, 2022

adomokos commented Aug 9, 2022

alessandro-fazzi commented Aug 11, 2022 •

edited

Loading

adomokos commented Aug 11, 2022

alessandro-fazzi commented Aug 12, 2022

adomokos commented Aug 12, 2022

adomokos commented Sep 8, 2024

Relax activesupport dependency #236

Relax activesupport dependency #236

Conversation

alessandro-fazzi commented Jun 27, 2022

adomokos commented Jun 28, 2022

adomokos Jun 28, 2022

Choose a reason for hiding this comment

alessandro-fazzi Jun 28, 2022 • edited Loading

Choose a reason for hiding this comment

adomokos commented Jun 28, 2022

alessandro-fazzi commented Jun 28, 2022

gee-forr commented Jun 29, 2022

gee-forr left a comment

Choose a reason for hiding this comment

gee-forr Jun 29, 2022

Choose a reason for hiding this comment

alessandro-fazzi Jun 29, 2022

Choose a reason for hiding this comment

gee-forr Jun 29, 2022

Choose a reason for hiding this comment

alessandro-fazzi Jun 29, 2022

Choose a reason for hiding this comment

gee-forr commented Jun 29, 2022

alessandro-fazzi commented Jun 29, 2022

alessandro-fazzi Jun 29, 2022

Choose a reason for hiding this comment

gee-forr Jun 29, 2022

Choose a reason for hiding this comment

adomokos commented Jun 29, 2022

alessandro-fazzi commented Jun 29, 2022

adomokos commented Jun 29, 2022

adomokos commented Aug 9, 2022

alessandro-fazzi commented Aug 11, 2022 • edited Loading

adomokos commented Aug 11, 2022

alessandro-fazzi commented Aug 12, 2022

adomokos commented Aug 12, 2022

adomokos commented Sep 8, 2024

alessandro-fazzi Jun 28, 2022 •

edited

Loading

alessandro-fazzi commented Aug 11, 2022 •

edited

Loading