dry-types icon indicating copy to clipboard operation
dry-types copied to clipboard

Params namespace disapearing

Open sdalu opened this issue 3 years ago • 9 comments

Describe the bug

It is not possible to add new type definition in the Params namespace of included Types() using the following construction, (it is certainly the same for Coercible, ...)

To Reproduce

module Types
  include Dry.Types()
  
  module Params
    PageSize = Types::Params::Integer
  end
end

Getting:

(irb):39:in `module:Params’: uninitialized constant Types::Params::Integer (NameError)

sdalu avatar May 07 '21 08:05 sdalu

It's not a bug, it's just how ruby works. You'll get the same results with something like

GemTypes = Dry.Types()
GemTypes::Params::Integer # => returns type, ok

module Types
  include GemTypes
  # here Params::Integer is still available 
  # because it's inherited from GemTypes

  module Params # this shadows Params of GemTypes
    # now Types::Params references to another module, not one defined in GemTypes
  end
end

There are a bunch of workarounds you can apply. Simplest:

# don't use inheritence
Types = Dry.Types() 
# instead, work with the module created by dry-types directly
module Types
  module Params
    PageSize = Types::Params::Integer
  end
end

Another option is avoiding shadowing:

module Types
  include Dry.Types()

  Params::PageSize = Params::Integer
end

or

module Types
  types = Dry::Types().tap { include _1 }

  module types::Params
    PageSize = Types::Params::Integer
  end
end

or

module Types
  include Dry::Types()

  module ancestors[1]::Params
    PageSize = Types::Params::Integer
  end
end

flash-gordon avatar May 07 '21 12:05 flash-gordon

Wait, this is counter-intuitive. I would expect this to work. Given that Types::Params works, why re-opening the module does not?

solnic avatar May 07 '21 14:05 solnic

@solnic because it's not reopening the module but creating a new one. We can't control it. We could change our examples to Types = Dry::Types(), it'll help somewhat.

flash-gordon avatar May 07 '21 14:05 flash-gordon

@flash-gordon do you know if it's possible to define regular modules for Param JSON etc.? It feels like we're violating POLS here.

solnic avatar May 11 '21 07:05 solnic

@solnic I'm pretty sure you can subscribe to the included hook and define modules there. This would be surprising to me OTOH because it has nothing to do with dry-types rather how ruby operates itself. I've once heard of a framework that tries to improve ruby, not sure they really pulled this off. I suggest updating docs to Types = Dry.Types() and call it a day. Same effect, no magic. Keep in mind, lexical scoping in ruby is far from being self-evident. For instance, you can't make it work with either approach:

RSpec.describe 'foo' do
  include Dry.Types

  specify do
    Params # boom, no params!
  end
end

Oh! I have another suggestion actually! We can detect when the user overrides exported modules and give a proper warning. WDYT? This way we won't bend ruby (with unforeseeable consequences) and provide robust UX.

flash-gordon avatar May 11 '21 08:05 flash-gordon

I've been bitten by this before too, ended up scratching my head for a bit and then went for one of the workarounds.

I think if we follow this recommendation of @flash-gordon's:

I suggest updating docs to Types = Dry.Types() and call it a day. Same effect, no magic.

We'd effectively solve this issue for 99.9% of our users.

Is this the one you're suggesting, @flash-gordon?

# don't use inheritence
Types = Dry.Types() 
# instead, work with the module created by dry-types directly
module Types
  module Params
    PageSize = Types::Params::Integer
  end
end

That said, this suggestion does sound good too, if we're able to do it :)

We can detect when the user overrides exported modules and give a proper warning. WDYT?

timriley avatar May 11 '21 13:05 timriley

Is this the one you're suggesting, @flash-gordon?

Yep.

I think applying both improvements is a good way of solving this.

flash-gordon avatar May 11 '21 18:05 flash-gordon

Yeah let's just fix it via docs and:

We can detect when the user overrides exported modules and give a proper warning. WDYT?

sounds good to me too.

solnic avatar May 12 '21 07:05 solnic

hm, it's not possible to detect if there was a constant definition, not without tracepoint API at least, that'd be overkill FWIW https://bugs.ruby-lang.org/issues/17881

flash-gordon avatar Jul 25 '21 12:07 flash-gordon