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

GlobalID errors #126

Open
Matt-Yorkley opened this issue Feb 8, 2022 · 13 comments
Open

GlobalID errors #126

Matt-Yorkley opened this issue Feb 8, 2022 · 13 comments

Comments

@Matt-Yorkley
Copy link
Contributor

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

  • Create a fresh app with Rails 7.0.1 and Ruby 3.0.3
  • Add Futurism
  • Try futurizing a partial
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

  • Gem: 1.2.0.pre9
  • Node package: 1.2.0-pre9

External tools

  • Ruby: 3.0.3
  • Rails: 7.0.1
  • CableReady: 5.0.0-pre8
@julianrubisch
Copy link
Contributor

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

Will investigate...

@julianrubisch
Copy link
Contributor

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

@Matt-Yorkley
Copy link
Contributor Author

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. 🤔

@julianrubisch
Copy link
Contributor

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

@Matt-Yorkley
Copy link
Contributor Author

Matt-Yorkley commented Feb 9, 2022

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

Great find 👏🏻

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

@julianrubisch
Copy link
Contributor

@Matt-Yorkley
Copy link
Contributor Author

Confirmed adding require "global_id" in here resolves it:

require "rails"
require "action_cable"
require "cable_ready"

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

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 stimulusreflex/cable_ready#175, thus excluding ActionMailbox from the slug size, for example...

@rickychilcott
Copy link
Contributor

rickychilcott commented Feb 10, 2022

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

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

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

@julianrubisch
Copy link
Contributor

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.

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

No branches or pull requests

3 participants