anycable / graphql-anycable

A drop-in replacement for GraphQL ActionCable subscriptions. Works with AnyCable.
MIT License
107 stars 16 forks source link

Delay the deprecation warning of `use_client_provided_uniq_id` #26

Closed gsamokovarov closed 2 years ago

gsamokovarov commented 2 years ago

When setting the use_client_provided_uniq_id to false through Ruby configuration code the deprecation warning was still issued in our project:

GraphQL::AnyCable.config.use_client_provided_uniq_id = false

# or...

GraphQL::AnyCable.configure do |config|
  config.use_client_provided_uniq_id = false
end

This happened because the deprecation warning was issued through an on_load callback that is invoked when the GraphQL::AnyCable.config is initialised. At that time, Anyway is getting settings through sources like ENV variables, but the Ruby configuration code is invoked afterwards. At Dext we prefer having this setting in Ruby code.

This solution delays the deprecation warning until the Rails application is initialized.

Envek commented 2 years ago

Thanks for the report and for the solution.

However, non-Rails users won't get deprecation warning then… :thinking:

@palkan, what would be a correct way to resolve this for both Rails and non-Rails users?

gsamokovarov commented 2 years ago

I did a quickfix here. 😅

A general fix for this should live in anyway_config. It's tricky, because Anyway::Config#load is called on config's initialization. Maybe exposing interface like Anyway::Config.configure that first yields the Ruby config, then invokes the on_load callbacks can work, but the other setting sources should be invoked before the Ruby code.

palkan commented 2 years ago

Maybe we can move deprecation into the use method: https://github.com/anycable/graphql-anycable/blob/736501cedd9820f3c481c6fd3b8da2c1b605f8be/lib/graphql-anycable.rb#L13

This method is expected to be called after application configuration.

Maybe exposing interface like Anyway::Config.configure that first yields the Ruby config, then invokes the on_load callbacks can work, but the other setting sources should be invoked before the Ruby code

Yeah, that's a good idea! Thanks!

I think, we can actually make it possible to pass a block to Config.new and execute right before load callbacks.

gsamokovarov commented 2 years ago

Thanks for the suggestions of issuing the deprecation in the use method. I have moved it there in this change.

How do you want proceed with this change? Do you folks prefer to solve the problem at its core or are you fine with moving the deprecation and fixing anyway_config at its own pace?

palkan commented 2 years ago

I think, we should merge the current version; it's better to handle this in this lib than to depend on third-parties.

Envek commented 2 years ago

Released in 1.1.4. Thanks for the pull request!

gsamokovarov commented 2 years ago

Thank you for the release and sorry to bring it back up. I thought I have applied Vlad's suggestion in this PR, but I have forgotten to push it. I have done it in another PR, if you want to decouple this warning from rails: https://github.com/anycable/graphql-anycable/pull/27.