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

Stronger checks for existence of config gem #1802

Merged
merged 1 commit into from
Feb 22, 2024

Conversation

paracycle
Copy link
Member

Motivation

Solve a problem experienced with a Shopify project.

Just checking for the existence of a Config constant does not necessarily guarantee that the config gem is loaded, since an application could define their own top-level Config constant.

Implementation

It is better to check for the existence of Config::VERSION and Config.const_name as well, to be reasonably sure that the constant is coming from the gem.

Tests

All existing tests pass

Just checking for the existence of a `Config` constant does not necessarily guarantee that the `config` gem is loaded, since an application could define their own top-level `Config` constant.

It is better to check for the existence of `Config::VERSION` and `Config.const_name` as well, to be reasonably sure that the constant is coming from the gem.
@@ -1,7 +1,7 @@
# typed: strict
# frozen_string_literal: true

return unless defined?(Config)
return unless defined?(Config) && defined?(Config::VERSION) && defined?(Config.const_name)

Choose a reason for hiding this comment

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

this might be just kicking the can down the road a bit 🤔 Long term does the constant need to be moved to a namespace?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is not a constant from Tapioca. This is a compiler for the config gem and this line is trying to detect if the config gem is loaded or not: https://github.com/rubyconfig/config

So, we can't rename the constant. This is the best way that I know if checking if the config gem is loaded without false positives.

Choose a reason for hiding this comment

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

interesting. thanks for the explanation. This is a rather precious namespace for a dependency of a dependency to eat 🤔

options:

  • fork & rename
  • rename at runtime
irb(main):001:0> module Bob; end
=> nil
irb(main):002:0> Bob
=> Bob
irb(main):003:0> Henry = Bob
=> Bob
irb(main):004:0> Henry
=> Bob
irb(main):006:0> Object.send(:remove_const, :Bob)
=> Bob
irb(main):007:0> Henry
=> Bob
irb(main):008:0> Bob
Traceback (most recent call last):
        4: from /usr/bin/irb:23:in `<main>'
        3: from /usr/bin/irb:23:in `load'
        2: from /Library/Ruby/Gems/2.6.0/gems/irb-1.0.0/exe/irb:11:in `<top (required)>'
        1: from (irb):8
NameError (uninitialized constant Bob)
irb(main):009:0> 
  • remove this dependency, find another way to load configuration files (if it's a light enough lift, probably preferable)

just some ideas. The metaprogramming idea might be tricky depending on loading order.

Copy link
Member Author

Choose a reason for hiding this comment

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

Again, this is not a dependency Tapioca brings in, Tapioca just tries to do the right thing for apps that might be using this gem. We have no control over how applications use this gem, so we can't do any runtime manipulations.

Choose a reason for hiding this comment

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

gotcha, I misunderstood.

Copy link
Member

@alexcoco alexcoco left a comment

Choose a reason for hiding this comment

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

Looks good to me to fix the issue but I do agree that maybe this ought to be in a namespace owned by Tapioca.

@paracycle
Copy link
Member Author

Looks good to me to fix the issue but I do agree that maybe this ought to be in a namespace owned by Tapioca.

#1802 (comment)

@paracycle paracycle merged commit 82794e1 into main Feb 22, 2024
34 of 35 checks passed
@paracycle paracycle deleted the uk-stronger-config-gem-checks branch February 22, 2024 20:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants