Add convenience middleware that uses renderContext to add `req.user` to the context for all templates

[mjreich]

This would be exposed as user in the template.

[mjreich]

@loppear I was thinking about this one - don’t we expose this to a race condition where you potentially have a collision between different render processes for different users? It seems like there are places in a site with high request volume where two users requesting a page simultaneously would see their request objects switched (middleware sets request for UserA then UserB, but UserB’s render loop resolves before UserA’s for some reason)

[loppear]

Oh bother, yes. There have been other times where I wanted a request-specific event for this, and I don't know why I thought this case was different.

[loppear]

So the goal here for me is that page level (theme-level) parts of the template can count on having access to req or user (for a Profile header or whatever) without every request handler being able to quietly break that by forgetting to pass req or user to the templater.render call. But it's true that the only place where the context and the req are both in scope is in that handler, not middleware.

[mjreich]

@loppear the current implementation is certainly bunk, but I wonder if we change it to:

    router.middleware((req, res, next) => {
      templater.on('renderContext', () => {
        return {req}
      })
      next()
    })

That would tighten things up with regards to the context, but still could introduce a race condition if the requests are coming fast enough?

Also, using templater.on is going to cause a memory leak.

I guess I really don’t see a way to avoid this without the route handler providing some reference to the current request.

[loppear]

(that one definitely has the same race problem)

Possible approach would be templater middleware that adds a method to response (as passport adds user to request) for rendering templates. Handlers wouldn't call templater.render but res.template(). And then you have a place for request/response-specific renderContext to be registered (res.renderContext) by other middleware (probably don't even need it to be an event, just an object that gets merged by the implementation of res.template). And then no worries about memory leaks from per-request listeners. (Site-wide uses of templater.on as for includeScript fine, don't multiply and leak.)

But we'd have to mull on whether moving these helpers onto response is a good pattern.

[mjreich]

@loppear yeah, not sure I'm wild about that vs passing req as a best practice from the handler. Regardless of the approach, templates always need to be checking variable typeof before use as to not throw exceptions if undefined.

[loppear]

Agreed, happy to postpone this to a later version after trying out some of these possibilities for an app or two, and agree that none of the choices change how templates themselves are implemented. With this discussion, I'm going to close the issue as the specific "req.user template middleware" is definitely not happening in any of these possible futures. :)