I'm okay with this, but I do wonder if it's exposing too much of our internals. However, that ship has probably already sailed, so being more defined with the return value is probably okay.

@jeremyevans wdyt?

Thanks Samuel for the comment. I think a proc that just returns an instantiator for the requested class with the given arguments is what the user would expect and want, and does't expose anything likely to change.

The @use array however....

This change isn't needed for OmniAuth, but without it it would have to create its own procs.

If we are considering making a backwards incompatible change to the return value of use, I think it should return nil. Either the current or proposed changes return values that should be considered implementation details. Changing to nil makes it so we don't leak implementation details.

In general, I don't think @use being an array of procs is the best design choice. I think the only reason procs are used currently is to combine the middleware and mapping features. That doesn't seem like a great idea to me, and I don't want to be unable to improve the internals because we exposed implementation details that didn't need to be exposed.

It's probably better if Omniauth creates it's own procs.

Thanks Jeremy.

Isn't the advantage of using procs that it doesn't waste time and memory creating a middleware until it's needed on a request, especially valuable in development mode.

To do the same in OmniAuth I was planning something close to

@strategies = {}
@strategy_creators = {}

def provider(klass, *args, **opts, &block)
  ...
  creator = use(middleware, *args, **options.merge(opts), &block)
  @strategy_creators[klass] = creator
  creator
end

def self.get_strategy(name) {
  unless @strategies[name] ||= @strategy_creators[name]&.call
    raise "No in-use strategy #{name.inspect}"
  end
end

There's also a need for Rack itself to allow middleware lookups, which would be an alternative to the above, and could be done even if use returned nil, by adding new lookup method that returned a proc or a called proc.

IMHO, you shouldn't be using global state for this.

It would be better to create a container of strategies (e.g. OmniAuth::Strategies or OmniAuth::Configuration), and then create middleware from that.

I think you're right Samuel that aggregation of strategies is the better way, allowing individual strategy instances to be cached and looked-up internally. Unlike some middleware, strategies don't need to be continually re-instantiated/reset.

I was trying to get away with minimal changes to more easily sell them to OmniAuth. But it looks again like half-arseing just defers the pain.

like half-arseing just defers the pain.

I know your pain.

Isn't the advantage of using procs that it doesn't waste time and memory creating a middleware until it's needed on a request, especially valuable in development mode.

I don't believe so. I think procs are only used here to combine the handling of maps and middleware. The procs are only supposed to be used by to_app when building the app.

There's also a need for Rack itself to allow middleware lookups, which would be an alternative to the above, and could be done even if use returned nil, by adding new lookup method that returned a proc or a called proc.

This would be even easier if use was an array of arrays instead of an array of procs. You could even provide more features in that case, such as the arguments to each middleware, which you cannot get with the proc approach.

After discussion, it looks like we aren't going to merge this change. Thanks for your effort.