[AutoLoad] Autoload Fixes #1904
Conversation
| if never_mounted? | ||
| base_instance | ||
| else | ||
| mounted_instances.first |
There was a problem hiding this comment.
Should this be all instances and the method to return an array?
There was a problem hiding this comment.
hmm. I can't remember why this should ever return one of the mounted instances, as you probably want to always be interacting with rack on a predictable instance. I'll have to see what happens if I have this always be the base_instance
There was a problem hiding this comment.
OK so this change was initially introduced in: https://github.com/ruby-grape/grape/pull/1893/files
The reason why this is needed is that we used to support - before remounting was introduced - the ability to pre-mount the API you will run in a namespace (and I was maintaing backwards compatibility).
Basically you are allowed to do
class ToBeMountedinRack < Grape::API
get 'endpoint'
end
class Namespace < Grape::API
namespace 'namespace' do
mount ToBeMountedinRack
end
end
run ToBeMountedinRackAnd the expectation is that you will be able to get /namespace/endpoint
I am not sure I want to maintain this behaviour forever, as it doesn't really play nicely with re-mounting an endpoint and I don't find it very sensible (I would find it sensible to run Namespace if you want the above behavour, rather than mounting ToBeMountedinRack)
Nonetheless, I don't think this is the appropiate PR to introduce this change
There was a problem hiding this comment.
Agreed. We can deprecate this separately.
|
Ok, I'm good with this. Let's add a test? Something that calls |
|
I merged this. |
|
|
||
| it 'compiles the instance for rack!' do | ||
| stubbed_object = double(:instance_for_rack) | ||
| allow(app).to receive(:instance_for_rack) { stubbed_object } |
There was a problem hiding this comment.
@myxoh I am curious about the purpose of this test case. It seems to test nothing?
* [Autoload] allows all bits of Grape to be autoloaded * [AutoLoad] creates a compile! method to pre load an API class * [AutoLoad] Addresses rubocop rules, and adds a readme line * [AutoLoad] Adds changes to changelog * Addresses issues with danger * Makes sure the check happens inside the Mutex * Syntax on the README * [AutoLoad] does not take the lock to try to compile is an instance is already present * [AutoLoad] Tests the new methods compile! and instance_for_rack
Partially addresses the issues raises on: #1902
The way it does so is it allows a method
compile!that makes a large percentage of the work to warm up the API on load time.More work is needed before the API is 100% warmed up.
Taking suggestions as to how best to test this