Repair Rails compatibility - Revert "Flatten Rack::Request::Env into Request"

[agrberg]

reverts commit d96b5c3

[tenderlove]

FWIW I think we should fully revert #1751. The Env module should call super because we don't know where it will be included. Other modules / classes above it in the inheritance tree could need initialize to be called, and if we remove the super here, it won't be called.

[agrberg]

@tenderlove updated w/ a comment to forewarn others from this journey 😅

[jeremyevans]

This mostly looks good. I have a couple suggested changes.

[jeremyevans]

Code change looks good, I think the CHANGELOG line should be removed, not modified.

[ioquatix]

We are allowed to make breaking changes for Rack 3. According to https://github.com/rails/rails/blob/d5f517c136858e83adc209775416176ca5dda677/actionpack/actionpack.gemspec#L37 ActionPack should be able to transition appropriately.

Regarding this change, I would say it's somewhat risky for ActionPack to pull in a module like this. Adding or changing methods may be problematic too, e.g. if we introduce a method which clobbers an existing one in Rails.

[tenderlove]

Yes, we are allowed to make breaking changes in Rack 3, but that doesn't mean we need to. Removing this module just for the sake of removing the module and a call to super doesn't seem worth forcing Rails to change. Practically speaking it would mean Rails would essentially just copy/paste code from Rack in order to maintain compatibility, and that isn't good either.

FWIW I'm kind of happy about the flexibility a module provides. It means we can provide a way for people to compose a "Rack like" request object without forcing people to inherit from our Rack request class.

[tenderlove]

Actually we removed the super call in Rack::Request, so we'd just be removing the module for the sake of removing the module. Doesn't seem worth it.

[ioquatix]

I'm easy either way, but my preference is towards composition rather than inheritance since I think it's easier to manage complexity. Therefore, I'd say we should be careful about the kind of design we encourage.

[agrberg]

tl;dr

My goals was to make the code easier to understand for the next reader. I value clear, explicit code and community patterns (❤️ Rubocop #SorryNotSorry 😅). When something doesn't match, my gut says the author did it for a reason. I didn't have enough knowledge about Rack to have an opinion.

I think we should merge this in its current state. It gets us back to before I opened my big mouth editor, adds comments to help the next person, and makes intent clearer by skipping the extra super(env) call.

My 2¢ (adjusted for inflation, w/ major caveat that much is shaped by the limits of my experience)

Including a module defined within another class within another gem/library like Rack::Request::Env and Rack::Request::Helpers in addition to having modules tap into the object instantiation are new to me. I'm must more familiar with modules that are explicitly intended to be included such as testing helpers like ActiveSupport::Testing::TimeHelpers or augmenters like the dry-rb tools.

Additionally, I'm more familiar with constructing a facade around an object I'd like to extend/coordinate with that I don't own. I didn't dive too deeply but I wonder why ActionDispatch::Request doesn't delegate methods to an encapsulated instance of Rack::Request rather than mixing in the two modules as this would provide explicit control over initialization.

So what's the path forward for the long term? I'm not sure. I want to make time my schedule to read/learn more about ActionDispatch::Request and see if a composed approached via a facade or adapter would work. Minutes of discussion can also save hours of coding so I'd like to see what y'all think about that as a plan.

[jeremyevans]

Looking at the history, Env was added specifically to deal with legacy Request objects that are ENV based but don't want to inherit from Rack::Request (b2d7396). That seems reasonable, and as it was added specifically for external use, it should be considered public API, which we shouldn't break without a good reason. So I'm in favor of merging this.

@ioquatix I think the argument for composition vs inheritance doesn't really apply here, if the choice is between having a separate Env module or just having the methods as part of Request. Is there an alternative approach you are advocating that uses composition for what the Env module handles? If so, can we merge this to restore backwards compatibility, and then discuss a more composition-based approach?

[ioquatix]

If this is supposed to be a mixin, why do we even define initialize?

Surely this is up to the consumer to set @env.

[agrberg]

Agreed that it's not what I expected when I entered the code base. I think things organically evolved as opposed to a coordinated conversation around how Rack's core is intended to be used/extended and with which patterns and assumptions.

This isn't my show so I'd be interested to hear how Rack's core team would intend/prefer the components of it to be extended. Ruby offers some powerful but complex inheritance and mixing tools that are fun to use, but I always prefer explicit encapsulation as I tend to not remember fancier programming after 3 months of absence.

[ioquatix]

Does it break Rails if we remove it?

[jeremyevans]

I'm OK with this as well. Since it looks like @ioquatix and @tenderlove already expressed they are in favor, I'm going to merge this.