[kbnServer/extensions] formalize request factories with helper #12697
Conversation
spalger
added
Team:Core
|
I think this may be premature. From a complexity standpoint, this is a 50 line abstraction to avoid repeating 4 lines of very-readable code, and it's a new abstraction in a request-oriented architecture that we don't necessarily plan to repeat in the new platform. Personally, I'd prefer that we explore an abstraction like this after we reuse the existing convention a couple times and realize it is actually causing us problems. I could be swayed though, so I'm interested in what others think on this. |
|
I'm not really sure what I think about this abstraction in general. All kinds of caching has a tendency to create subtle bugs. It also "hides complexity" in a way I'm not sure I like. The more I think about it the more I lean towards injecting the request (or the request specific object, like a request-scoped However, as mentioned, I'm not really sure what I think yet. I've got to explore these things a bit. But that's something we can tackle in a more focused manner in the new platform work, so I'm okey with this for now, if others prefer this. |
|
I strongly agree with @kjbekkelund on general caching concern, but if we decide (decided?) that we really need caching in several different places, I'd rather prefer to handle it in a single place like in this PR.
Strictly speaking half of those 50 lines are jsdoc/comments that is great and doesn't increase complexity :) All in all adding of reasonable amount of complexity doesn't concern me too much as long as it's sufficiently covered with unit tests that seems to be the case here. |
|
Talked with @kjbekkelund and @epixa offline who agreed we can move forward with this in master and we'll explore more desirable directions in the context of the new platform once we get to more in-depth http stuff there. So this is ready for review again |
|
|
||
| const requestCache = new WeakMap(); | ||
| server.decorate('request', methodName, function () { | ||
| if (!requestCache.has(this)) { |
There was a problem hiding this comment.
Is this the only way to do this? No other access to the request?
There was a problem hiding this comment.
If not, maybe const request = this above this line? Just to make it super-obvious.
With #12243 request's got a new
request.getUiSettingsService()method that returned a new instance of theUiSettingsServicethat was already configured for that request based on the request context. This method wasn't terribly expensive, but sinceuiSettingsare necessary in several places on almost every request the factory was cached per request; subsequent calls torequest.getUiSettingsService()return the same return value as the first call.This was implemented one-off in the specific request extension, but with #12625 we're adding another method to requests,
request.getFieldFormatsService()that I think should have similar characteristics. Rather than re-implement the caching in a handful of places I figured we could formalize the pattern by creating a helper that future devs reading the code will be able to see and reuse for similar needs.