stimulusreflex / futurism

Lazy-load Rails partials via CableReady
MIT License
407 stars 19 forks source link

GlobalID errors #126

Open Matt-Yorkley opened 2 years ago

Matt-Yorkley commented 2 years ago

Bug Report

Describe the bug

Trying out Futurism on a fresh app I'm getting fatal errors that seem to be a class-loading issue with GlobalID.

To Reproduce

NameError - uninitialized constant Futurism::Resolver::Resources::GlobalID

The backtrace points to:

[...]/3.0.0/gems/futurism-1.2.0.pre9/lib/futurism/resolver/resources.rb:92:in `resolved_models' 

I notice in my Gemfile.lock I have globalid at version 1.0.0, which was added by default. If I downgrade and manually pin it to <= 0.6.0 instead, the issue is resolved.

Versions

Futurism

External tools

julianrubisch commented 2 years ago

Huh that is weird, I haven't run into this in spite of doing a couple of fresh installs lately.

Will investigate...

julianrubisch commented 2 years ago

It's even more weird because the GlobalID changelog mentions no code changes between 0.6.0 and 1.0.0 🤔

Matt-Yorkley commented 2 years ago

Results of debugging round two: it actually does work with 1.0.0, but only when I have globalid explicitly declared in my Gemfile. If I remove it (even though it's still in my Gemfile.lock and apparently present) I get the error. :thinking:

julianrubisch commented 2 years ago

Oh... imho that's even stranger... could you prepare an MVCE?

Matt-Yorkley commented 2 years ago

Ahaaaa... okay I think I got it. Results of debugging round three:

The app I was working on was an MVP so I skipped a lot of modules when doing rails new.

Looking at the Gemfile.lock at which Rails modules use/depend on global_id, I can see it's action_text and active_job. I skipped / disabled both of those when creating the app.

Re-enabling either of those two core modules resolves the issue; because both of them explicitly contain require statements for global_id but Futurism doesn't.

julianrubisch commented 2 years ago

Great find 👏🏻

I think this would have surfaced earlier if futurism only included the respective rails submodules - which I intended to do, too 🙈

julianrubisch commented 2 years ago

Cf https://github.com/stimulusreflex/cable_ready/pull/175

Matt-Yorkley commented 2 years ago

Confirmed adding require "global_id" in here resolves it: https://github.com/stimulusreflex/futurism/blob/8d352a54a3f35bc2f7856774d6edefbd521d4fbf/lib/futurism.rb#L1-L3

Shall I PR it? It's a dependency of rails already, so it doesn't feel like it warrants a change in the gemspec. It just doesn't necessarily get required.

julianrubisch commented 2 years ago

yeah, the question I'd like to pose is if we should actually move away from requiring the whole of rails and just require the necessary gems...

there were some second thoughts in CR that led to https://github.com/stimulusreflex/cable_ready/pull/175, thus excluding ActionMailbox from the slug size, for example...

rickychilcott commented 2 years ago

I think it makes sense to only have dependencies on global_id (and require it) and cable_ready. We also have some dependencies on the controller code, activerecord, etc.

Do you think this list would do it?

  gem.add_dependency "actioncable"
  gem.add_dependency "actionpack"
  gem.add_dependency "actionview"
  gem.add_dependency "activerecord"
  gem.add_dependency "activesupport"
  gem.add_dependency "railties"
  gem.add_dependency "global_id"
  gem.add_dependency "cable_ready"
julianrubisch commented 2 years ago

Yep. I'm not sure if ActiveModel should be in there. It's probably in every application anyway so we could just make it explicit?

Matt-Yorkley commented 2 years ago

Is Futurism usable in non-Rails projects though? If it's not, then this extra granularity in the gemspec is moot, right?

julianrubisch commented 2 years ago

Not really, as pointed out in the referenced CR PR, because it blows up the memory slug size when you include action_mailbox and action_text by default, for example.