Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

View.prototype.lookup should be more easily overridable #1089

Closed
pierresforge opened this Issue · 8 comments

2 participants

@pierresforge

As of today, changing view lookup behavior requires modifying View.prototype.lookup. This looks dirty and, should the API change, not future-proof.

I would suggest adding a configuration option such as view lookup, accepting a function to use as a replacement for the default one, if needed.

This would make, for example, view lookup in multiple directories less dirty.

Additionally, it might be better for both performance and functionality (for example, storing views in database) to make the view lookup asynchronous.

I can do a pull request if necessary.

@tj
tj commented

im open to it being overridable, but you wouldn't really want to go grab things from a DB anyway, at least not per-request, that would be pretty slow. Making the signature sync would be fine now that we have an async signature for app.render/res.render

@tj
tj commented

i'll reference this from the other issue

@tj tj closed this
@tj
tj commented

another issue we'll have is the cache id, since you might want to cache view lookup based on various things, but if done incorrectly it would lead to pretty large memory leaks. An example might be an MVC view lookup so say "show" within a "user" controller would resolve to "./controllers/user/views/show.jade", and "show" in a "pet" controller may be entirely different, so the cache id would be user:show and pet:show

@pierresforge

First of all, I would say that referencing the same exact template with two different names is not really a good idea, and not really great code as well.

That said, I think we could imagine to cache the View only when there is a real win in doing it. In other words, only after the lookup is done? We could then use the absolute path returned from the lookup function as the cache key. This would at the same time solve your issue and improve cache efficiency: we would not rely completely on user input for choosing this cache key.

@pierresforge

Another option, which I would prefer in essence, but which is probably hard to implement today given its compatibility issues (4.x?), is to delegate completely the view lookup to the template engine.

With this modification, we would not need any cache in express for views, and we would avoid needing a lookup function both in express for views, and in the template engine for includes / layouts.
As most template engines are already caching compiled templates, this would also help grouping all the view-related cache in the only place that makes sense: in the template engine.

@tj
tj commented

It depends how your code is structured. If it's modular via little controller-like things, each with their own views, that's potentially good decoupled code. I think this is a fundamental issue with how people build apps today, but Express is not the right level to target that, I just want to facilitate this sort of modularity.

As far as caching goes we should definitely cache all view related IO, otherwise it's way too slow.

As far as engines go, there doesn't seem to be any consensus on when to cache, how to enable caching etc. For EJS and Jade I pass { cache: true }, but some enable it at all times making dev-editing impossible. Lookup in the engine would be ok but that's one more thing an engine has to do in order to work reasonably well with Express. IMO having lookup in Express at least brings some engine-level familiarity as well which I think is nice. If all engines had some nice { root: path } option or similar I'd probably say yes to that but they're all way too different right now

@pierresforge

Actually, I think there was a pretty huge flaw in my thoughts for the first part. My mistake.

Thinking about this again, my idea would be to build the cache key with the matched route and the template name. This is the most transparent solution I can think of, but I don't know how compatible it is with planned routing system changes. This will surely lead to a little increase in memory consumption, but as you don't generally have thousands of routes in a single express application, it shouldn't be an issue for most use cases.

For the rare cases where this could be a real issue, maybe we could make the cache key construction overridable as well. This would allow people with specific needs to tweak express caching behavior, and to build cache key in accordance with how their lookup function works to minimize memory footprint.

@tj
tj commented

yeah we could maybe get away with caching req.route.path + ':' + viewName or something. I dont think it's something too many people would use but I'd at least like to facilitate passing the same name to res.render() for that sort of modularity, but im also thinking that's a good use-case for mounting since you can have self-contained "views" settings

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.