Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

example/darkmode: add darkmode example #185

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

matthewmueller
Copy link
Contributor

@matthewmueller matthewmueller commented Jul 2, 2022

This PR reproduces theme flickering to understand what needs to change in bud:

  • Fix SSR flickering
    cookie.get(...) uses document.cookie under the hood. document.cookie is 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.

…ng to understand what needs to change in bud.
@matthewmueller
Copy link
Contributor Author

cc @vito in case you have any thoughts!

)

type Controller struct {
Writer http.ResponseWriter
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an over-simplification of a feature I recently fixed in Bud. See this PR for details.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@matthewmueller matthewmueller Jul 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@matthewmueller matthewmueller Jul 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants