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

Allow multiple instances with different setup #4

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

shimaore
Copy link
Contributor

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:

  • I didn't realign the code in index.js, to keep the patch as simple as possible; basically all the module-level functions become instance-level functions;
  • I updated the README, the changes in the case of a single Express server are very simple.
  • I added a separate test set to test for the case with multiple instances.

Instead of changing the settings at the module level, we allow the user to create a new instance of `partials` for each Express instance.
This avoid issues with conflicting settings when multiple Express engines are running concurrently.

In that case the usage would then be:

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

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

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

Without this patch, the latest `register('html',...')` called would override the options for both.
@slaskis
Copy link
Member

slaskis commented Jun 26, 2012

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Member

slaskis commented Jul 7, 2012

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
Copy link
Contributor Author

[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.

shimaore added a commit to zappajs/zappajs that referenced this pull request Jul 29, 2012
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