Conversation
…ng to understand what needs to change in bud.
|
cc @vito in case you have any thoughts! |
| ) | ||
|
|
||
| type Controller struct { | ||
| Writer http.ResponseWriter |
There was a problem hiding this comment.
This is an over-simplification of a feature I recently fixed in Bud. See this PR for details.
There was a problem hiding this comment.
Interesting - so a controller (or its dependencies) may depend on http.ResponseWriter and *http.Request and they'll be injected in on a per-request basis?
That's a neat way of keeping the action methods limited to request params.
I wonder if this comes with extra allocation cost or dependency construction churn though - for example, if I'm modeling my database as a dependency I probably wouldn't want it reconnecting with every request. Or does it only reconstruct dependencies as needed?
There was a problem hiding this comment.
I wonder if this comes with extra allocation cost or dependency construction churn though - for example, if I'm modeling my database as a dependency I probably wouldn't want it reconnecting with every request. Or does it only reconstruct dependencies as needed?
So the database client is able to be passed in once through all requests.
There's a concept called in the DI framework called hoisting that's able to externalize dependencies that don't depend on specific other dependencies. Hoisting just means, mark those dependencies as something you need to pass into the provider as well.
In the case of controllers, dependencies that depend on *http.Request or http.ResponseWriter are not hoistable, they need to be re-initialized every request. Every other dependency is.
It looks like this in the generated code:
// Handler function
func (i *IndexAction) handler(httpResponse http.ResponseWriter, httpRequest *http.Request) http.Handler {
controller, err := loadController(
i.Client,
i.Logger, httpRequest, httpResponse,
)
if err != nil {
return &response.Format{
JSON: response.Status(500).Set("Content-Type", "application/json").JSON(map[string]string{"error": err.Error()}),
}
}
handler := controller.Index
// Call the controller
in0 := handler()
// Respond
return &response.Format{
JSON: response.JSON(in0),
}
}
// Generated by di
func loadController(dbClient *db.Client, logLogger *log.Logger, httpRequest *http.Request, httpResponseWriter http.ResponseWriter) (*controller.Controller, error) {
sessionSession := session.New(logLogger, httpResponseWriter, httpRequest)
controllerController := &controller.Controller{Session: sessionSession, DB: dbClient}
return controllerController, nil
}Where i.Client and i.Logger don't depend on the request, so they're passed in rather than initialized by the DI provider. Session on the other hand does depend on the request, so it's initialized per request.
There's definitely an allocation cost though. Even without hoisting, we load the controller every time. If you're doing something expensive in your controller.Load() that would slow things down per request. Thinking about it now, I could see that being quite surprising. Actually, this may point in favor of passing request-scoped dependencies in.
I was indeed trying to avoid mixing dependencies with the API signature, but I think DI is smart enough at this point to be able to filter those keys out.
There was a problem hiding this comment.
Awesome, thanks for the explanation.
Actually, this may point in favor of passing request-scoped dependencies in.
That sounds nice. Do you mean something like this?:
func (c *Controller) Logout(session *Session) error {
session.Logout()
}To me this feels pretty intuitive since as you said they're 'request scoped' just like the params are, but I guess you'd need some way for Bud (and maybe the developer) to disambiguate dependencies from params. 🤔
Or maybe you don't? I may be off my rocker here but one way of looking at it is that params are a just another type of dependency that load their values from the *http.Request.
Whether that's a line you want to blur is another question though regarding UX.
There was a problem hiding this comment.
Or maybe you don't? I may be off my rocker here but one way of looking at it is that params are a just another type of dependency that load their values from the *http.Request.
This was my thought process at the time, but I'm definitely leaning more towards something like what you wrote now:
func (c *Controller) Logout(session *Session) error {
session.Logout()
}Which makes more intuitive sense because the lifecycle is more clear and doesn't suffer controller.Load() being potentially expensive. Bigger fish to fry right now, but I'll get back to this one before 1.0!
Persisted here: #211
This PR reproduces theme flickering to understand what needs to change in bud:
cookie.get(...)usesdocument.cookieunder the hood.document.cookieis not defined by V8 on the server-side, but probably should be. We'd need to take care exposing DOM APIs though. We don't want to get in a situation where people get confused by what's available on server-side and what's not.Trying to keep this PR as basic as possible so no AJAX or anything. All that stuff can be added on top.