Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Delay the deprecation warning of use_client_provided_uniq_id #26

Merged

Conversation

gsamokovarov
Copy link
Contributor

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.

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

```ruby
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, not
in an ENV or a YAML setting file.
@Envek Envek requested a review from palkan July 7, 2022 18:01
@Envek
Copy link
Member

Envek commented Jul 7, 2022

Thanks for the report and for the solution.

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

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

@gsamokovarov
Copy link
Contributor Author

gsamokovarov commented Jul 8, 2022

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
Copy link
Member

palkan commented Jul 13, 2022

Maybe we can move deprecation into the use method:

def self.use(schema, **options)

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
Copy link
Contributor Author

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
Copy link
Member

palkan commented Jul 21, 2022

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

@Envek Envek merged commit 1265973 into anycable:master Jul 28, 2022
Envek pushed a commit that referenced this pull request Jul 28, 2022
When setting the `use_client_provided_uniq_id` to false through Ruby
configuration code the deprecation warning was still issued:

```ruby
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**. But we prefer having this setting in Ruby code, not
in an ENV or a YAML setting file.
@Envek
Copy link
Member

Envek commented Jul 28, 2022

Released in 1.1.4. Thanks for the pull request!

@gsamokovarov
Copy link
Contributor Author

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: #27.

Envek added a commit that referenced this pull request May 7, 2024
…dling

* master:
  Switch to RubyGems Trusted publishing in CI release workflow [ci skip]
  add emergency actions TO README
  fix: consider redis_prefix in GraphQL::AnyCable::Cleaner
  add ability to set scan_count and added a little refactoring
  ability to fetch subscription stats
  use "around" instead "before"
  little refactoring
  add changes to README
  remove override redis_prefix and add an extra spec
  feat: Add configurable graphql redis_prefix
  Support empty operation name with redis.rb v5 (#34)
  Use arrays to fix deprecation warnings with redis gem 4.8 (#29)
  Use real Redis in tests instead of fakeredis gem (#32)
  Better `config.use_client_provided_uniq_id` deprecation (#27)
  Delay the deprecation warning of use_client_provided_uniq_id (#26)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants