Skip to content

Conversation

zzak
Copy link

@zzak zzak commented Nov 13, 2024

The goal here is to remove deep_merge and rely entirely on ActiveSupport's Hash#deep_merge implementation.

This does mean that if you want to keep the existing behavior, or rely on DeepMerge's specific options, you need to add the following to your Gemfile:

gem 'deep_merge', '~> 1.2', '>= 1.2.1'

@pkuczynski Opening this as a proof-of-concept, would you be willing to take on this type of change?

I decided not to touch the config methods, even though they are no-op when deep_merge gem isn't included. Wanted to get feedback first, and I figure the warning is enough.

The goal here is to remove deep_merge and rely entirely on
ActiveSupport's `Hash#deep_merge` implementation.

This does mean that if you want to keep the existing behavior, or rely
on DeepMerge's specific options, you need to add the following to your
Gemfile:

```ruby
gem 'deep_merge', '~> 1.2', '>= 1.2.1'
```
@zzak zzak force-pushed the soft-require-deep_merge branch from 60fcabd to 85d692e Compare November 13, 2024 09:01
@pkuczynski
Copy link
Member

I think it's fine to relay on ActiveSupport when someone works in Rails environment and does not use (or expects) certain behaviour from deep_merge. But to accept such change we would have to

  • ensure that user is on Rails and if not, throw error that he needs to install deep_merge
  • ensure that Hash#deep_merge supports all current default settings of config gem (backward compatibility)
  • throw error to the user when he uses an option for config requiring deep_merge
  • update documentation to explain which options work by default in Rails and which require deep_merge

All of that would be a good change (less deps is typically better), but require significant changes and I am not 100% sure if it's worth it?

@cjlarose what do you think?

@pkuczynski pkuczynski added this to the Next milestone Jul 9, 2025
@pkuczynski pkuczynski changed the title Make deep_merge a soft-requirement feat: make deep_merge a soft-requirement Jul 9, 2025
@pkuczynski pkuczynski modified the milestones: 5.6.0, Next Jul 11, 2025
@pkuczynski pkuczynski requested a review from Nuzair46 July 15, 2025 11:29
@pkuczynski
Copy link
Member

@Nuzair46 what do you think about this one?

begin
gem 'deep_merge', '~> 1.2', '>= 1.2.1'
require 'deep_merge/core'
rescue LoadError
Copy link
Member

@Nuzair46 Nuzair46 Jul 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think here like @pkuczynski said, it is better to raise an error if deep_merge is not available in a non Rails codebase. Since this gem is can be used in any ruby codebase, it is better to warn for Rails and raise error for non rails app if it is not available. Imo if ActiveSupport deep_merge cannot support all the features of this gem, it might not be worth modifying the code to disable those features. It might be worth keeping this dependency for better DX.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

3 participants