Difficult to use in development with Rails

[jeromegn]

Hey there, long-time fan of Virtus here. Trying to switch over to Dry::Data.

We're using Rails and it appears I keep stumbling on There is already an item registered with the key errors. I believe that's due to how Rails reload classes and such upon file changes.

The only way I found to circumvent that was to use:

Dry::Data.remove_instance_variable :@container

... to reset the container. However, I had to do that manually in the process.

Any idea how to simplify that?

[solnic]

Thanks for trying out dry-data, and I'm glad to know you found Virtus useful.

So, there's no way to fix this, except to provide a workaround, and what you do is exactly what I do. We could provide some "helper" interface that could be activated in rails env. In fact, we could have a dry-data-rails railtie that would provide some rails-specific stuff and maybe even some conventions, like a default "type namespace" module.

I'm marking this as help-wanted as I deliberately refuse to spend any extra time because of Rails-related issues, unless it affects me in a painful way. Sorry about that but I had to become more conservative due to my time constraints :(

I'm happy to discuss this further.

[jeromegn]

Alright, well I created a really simple gem to make this work.

https://github.com/jeromegn/dry-data-rails

Published as dry-data-rails on rubygems.org.

[solnic]

Thanks for that. I'm sure it's gonna be useful. One thing to mention is that it should only be needed in development mode, so when Rails.env.development? returns true.

I'll add information about your gem to this project's README.

[AMHOL]

Might be better to check with config.cache_classes rather than env.development?

[solnic]

IIRC to_prepare block should be sufficient

On Fri, 15 Jan 2016 at 00:13, Andy Holland notifications@github.com wrote:

Might be better to check with config.cache_classes rather than env.development?

— Reply to this email directly or view it on GitHub https://github.com/dryrb/dry-data/issues/18#issuecomment-171812688.

[AMHOL]

Yeah, should be OK, would run once in production and once per request in development according to docs, although it doesn't need to run at all in production as far as I can tell, I guess it won't do any harm.

[jeromegn]

Ah! I'm not sure if I should be changing something now. I'm concerned there might be unexpected behaviour from Rails in production even with just the one-time execution of to_prepare. I should see it soon enough!

I'm curious about the test environment too. I'm not entirely sure how Rails handle that. My guess is it'll all be fine. I'll see about that soon enough too.

Thanks for the feedback and including it in the readme.

[apotonick]

Is this because of Rails autoloading not erasing the class instance variables, or something else?

[jeromegn]

@apotonick Something like that, I don't exactly remember. It's been a while!

[solnic]

@apotonick this is a problem in any case where something is cached globally

[apotonick]

@solnic So this also happens outside of Rails? :thinking:

[solnic]

@apotonick no, I'm talking about rails, it's code reloading, and the fact that every developer who writes a gem, which uses some kind of object caching at class level, needs to take this into consideration if they want to make their gem work with Rails. I don't give a 💩 so it's broken in Rails.

[flash-gordon]

I bet that was a problem with automatic struct registrations or something like that. This issue is no more.

[solnic]

@flash-gordon yeah we used to store types in Dry::Data namespace (as in, module), we no longer do that, but OTOH we store constructor functions globally (right?) so, this may cause issues

[apotonick]

Yes, I know it's because of Rails! We have the same problem, but we could "fix" them by following the very limited Rails-way of naming, e.g.

# file path is app/concepts/memo/operation/create.rb
class Memo::Operation::Create # normally, this would be called Memo::Create
end

And then, either let it auto-load, or use

require_dependency "memo/operation/create"

Any other naming/loading will result in class instance variables not being erased properly.

I'm talking to @fxn about a possible fix in Rails, where we could say

require_dependency "memo/operation/create", "Memo::Create"

to have class names different from files (which is super annoying in Rails). I bet 100 bucks that is the same problem in Dry/Rails, expecially with the anonymous classes you're creating, right?

[apotonick]

@solnic I think the problem in Dry is that schemas aren't erased properly, that's why you have double indices etc. We have the exact same problem.

[flash-gordon]

@solnic oh, that's global indeed so yes, so the issue still holds, though now it leads to a mild memory leak in dev mode.

[solnic]

@apotonick I don't have time for this. People can write their integration gems for dry/rails, just like we have rom-rails, we can have dry-rails railtie which would take care of various problems.

[fxn]

I don't find mapping file names to class/module names "super annoying", I find it very intuitive actually, and a natural way to be able to autoload.

The flexibility point in Rails is top-level directories, you can have as many as you want, and if they are under app they are automatically added to the autoload path (you could have app/operations/memo/create.rb to have Memo::Create autoloaded).

Now, if you want to depart from this schema, that's your call, but then your are opting out from Rails autoloading and reloading as a consequence. And that is not going to change any time soon, you have to roll your own solution somehow.

[apotonick]

I'm simply asking you for help to make Rails more flexible. There are new frameworks that have a noticeable user base, and many of those keep having problems with Rails and its autoloading. One solution is "Don't use Rails!", the other is to discuss and extend Rails, that's all I'm trying to achieve.

[fxn]

Nick I would ask you to be careful with the language, please.

If feature F has a contract that says "F works given precondition X", and you do not comply with X, then F is not the problem, it is the lack of compliance! Also, F does not need to be "fixed".

The way to phrase this respectfully is: "These apps have deliberately chosen to not comply with X. I know they have put themselves in trouble by their own decision, but could Rails perhaps add support for this in the future?"

And if the answer was "no" for whatever reason, well, you (a generic you) chose the convention when it didn't work already.

[apotonick]

Xavier, sorry nothing I said was supposed to offend you or anyone?!?!

[fxn]

No, no, I am not personally offended.

[apotonick]

Ok, good! :relaxed: I'm only a bit frustrated sometimes because we've been trying to make software developing better for more than 10 yrs and every time we hit a Rails problem, it's always "our problem" (ActionView's messy code vs. the Cells gem, ActiveModel::Validations code not allowing Reform to hook in, etc etc).

I'm fine if you say "Rails is not interested in solving this", but it would be cool to solve this "lack of compliance" by making Rails more tolerant. Please also understand my frustration a tiny little bit, I'm not just a useless ranter over here :grimacing: :stuck_out_tongue_winking_eye:

[fxn]

Autoloading/reloading is somewhat tricky (as the extension of the autoloading guide suggests), and the ramifications of opening its contract have to be carefully pondered.

I have hacked something quickly so that TRB can add support for autoloading/reloading using its preferred naming conventions. It would be something like this:

def trb_require_dependency(file_name, constant_path)
  loaded = require_dependency(file_name)

  if loaded && !Rails.application.config.cache_classes
    if eval "defined?(#{constant_path})"
      ActiveSupport::Dependencies.autoloaded_constants << constant_path
    else
      raise "#{file_name}.rb was expected to define #{constant_path}"
    end
  end

  loaded
end

Usage:

trb_require_dependency "memo/operation/create", "Memo::Create"

If existing applications throw that in an initializer I think it would mostly work.

That is a draft so that you see the main pieces in play. You could then adapt, of course. I don't know, the interface could be of a higher level like

require_operation "Memo::Create"

that behind the scenes builds paths or whatever and ultimately calls the low level trb_require_dependency, you know.

Hope that helps.

[apotonick]

Whoohoo, thanks brother! I can't wait to add support for this feature of Rails! :joy: Will report back soon, thanks again!!! :beers: :kissing_cat:

[flash-gordon]

as a side note, eval "defined?(#{constant_path})" can be replaced with Object.const_defined?(constant_path) I guess

[solnic]

I don't find mapping file names to class/module names "super annoying", I find it very intuitive actually

True. We do the same thing in apps based on dry-system, except that we don't use auto-load.

[fxn]

as a side note, eval "defined?(#{constant_path})" can be replaced with Object.const_defined?(constant_path) I guess

@flash-gordon They are not really equivalent, Object.const_get on paths with more than one segment is implemented as a recursive const_get basically. In particular it triggers const_missing on its way. defined? does not do that and for that test I find it to be more precise.

That explains why I wrote it that way, but in any case that snippet is just a draft/starting point, if you prefer Object.const_get and accept the trade-off that is fine too.

[fxn]

Ah, let me link that to Object.const_defined?.

Say you have

module A
  X = 1
end

module B
  include A
end

Compare:

B::X                          # => 1
Object.const_defined?("B::X") # => false
defined?(B::X)                # => "constant"

so if you want to use that API and get something that kinda works similarly, you need to manually imitate fetching the objects stored in the leading constants (dependencies.rb has needed historically to do some stuff by hand like that). And at that point you hit differences still like const_missing. Bottom line: as fas as I can tell the constants API is not able to implement defined?(Foo::Bar) in a truly equivalent way.

Just to clarify the reply above, same remark to leave the choice to you :).

[flash-gordon]

@fxn that's a very good point, thanks for sharing