publicclass / express-partials

Express 3.x Layout & Partial support.
182 stars 146 forks source link

Allow multiple instances with different setup #4

Open shimaore opened 12 years ago

shimaore commented 12 years ago

The implementation I proposed earlier doesn't deal well with different setup for views and partials using multiple Express processes in parallel.

Here's a simplified example:

var express_partials = require('express-partials');

/* server `express1` uses jade */
express1.set('view engine','jade');
partials1 = express_partials();
partials1.register('html','jade'); /* .html files are actually jade files */
express1.use(partials1);

/* server `express2` uses ejs */
express2.set('view engine','ejs');
partials2 = express_partials();
partials2.register('html','ejs'); /* .html files are actually ejs files */
express2.use(partials1);

Previously, the last register('html',...') called would override the options for both, breaking the other server.

Notes:

slaskis commented 12 years ago

Right, it's probably a good idea to not make those extensions "global". .html is a good reason, although why someone would mix their jade and ejs templates with an .html extension i don't know...

But, hmm...maybe there is a way we can use express' own register for this instead?

That way it would be nicely contained on an app-basis and we don't have to duplicate more code than necessary (i.e. the lookup code for _partial support).

Just an idea...

shimaore commented 12 years ago

But, hmm...maybe there is a way we can use express' own register for this instead?

That'd be engine instead of register, but yes, an Express instance will have an engines object that maps .ext to the renderFile function for that extension. The profle is renderFile(path,options,callback = function(err,str)).

Looking more closely at the code for express-partials, actually I see in the comments:

Render `view` partial with the given `options`. Optionally a
callback `fn(err, str)` may be passed instead of writing to
the socket.

This makes it sound like the partial function is async; however looking over its code it looks like it's sync.

So yes, if we rewrite partial as an async function we should be able to use the native render function and not have to deal with register, lookup, etc.

shimaore commented 12 years ago

Rewriting partial as async and seeing the tests fail reminded me why I didn't go that route: the way partial is used it must return the rendered template as string. Duh.

So if we want to make it work that way we'll have to look into e.g. node-fibers.

FWIW my (untested) attempt is in my async-partial branch -- I probably got the loop as async pattern wrong anyhow.

slaskis commented 12 years ago

Yes, one of the biggest changes since express 2.x (i think) is that the views are expected to be async. Even if they're not, in which they're wrapped in a async-like function.

And writing a partial that takes async is not impossible (you're half-way there it looks like) but right now most of the template engines seem sync anyway which I guess is why it hasn't been "solved" yet.

But what I was thinking about was that a lot of that registering-of-extensions seems to be duplicated and that it must be a way to do this better. Like using req.app.engines[ext] or something? Or do we really need to separate the engines?

Appreciate your input though!

shimaore commented 12 years ago

[sorry for the late followup, was out for a while]

But what I was thinking about was that a lot of that registering-of-extensions seems to be duplicated and that it must be a way to do this better. Like using req.app.engines[ext] or something?

engines only provides asynchronous rendering engines; to support partial we need synchronous rendering engines. register basically allows you to register synchronous engines, while engines allows you to register asynchronous engines. register is not used in most cases anyhow -- as long as the extension matches the name of the module for the rendering engine it will be loaded automatically, and the module's existing synchronous rendering method will be used.

That's a more generic discussion than the original topic of this issue, though. I was just trying to make sure the changes I proposed earlier would work if multiple concurrent Express instances were used.