jeresig / i18n-node-2

Lightweight simple translation module for node.js / express.js with dynamic json storage. Uses common __('...') syntax in app and templates.
MIT License
507 stars 79 forks source link

Fix usage of res.locals in Express 3.x #7

Closed sylvinus closed 11 years ago

sylvinus commented 11 years ago

This patch is needed to work with Express 3.x, tested on 3.0.5 and 3.1.0

I'm using ejs 0.8.3 as a template engine.

Resubmitting this as a separate PR

acstll commented 11 years ago

I can confirm this patch works.

Using lodash (underscore) templates and Express 3.1.0, while trying to use __() inside a template, I was getting this error:

node_modules/i18n-2/i18n.js:99
            return req.i18n[method].bind(req.i18n);
                           ^
TypeError: Cannot read property '__' of undefined

With this patch it now works.

@sylvinus I would appreciate a bit of explanation about the patch.

In my shallow understanding of the Express framework and JavaScript I can't understand where is the reference for the 'request' object, in the original code (line 96):

helpers[method] = function(req) {
    return req.i18n[method].bind(req.i18n);
};

I mean, if you're calling a function on res.locals, how is 'req' going to be passed…?

sylvinus commented 11 years ago

@acstll happy to hear that the patch works for you but I'm not sure I understand your question :-( Express changed the way they deal with res.locals in 3.x and all I did was adapt to that.

@jeresig will you be able to merge this and my other PRs ? I don't want to do i18n-node-3 ;-)

acstll commented 11 years ago

@sylvinus I'm sorry, my question was indeed blurry.

I do understand the code in the patch. You pass 'req' together with 'res.locals' when calling i18n.registerMethods(), so you can assign the functions in req.i18n to res.locals (binding this to req.i18n to make it work properly).

When rendering a view, all props and methods on res.locals (as well on app.locals) get merged into the object that gets passed as the 'data' object into whatever template engine does the job.

So, how was this going to work?:

res.locals.__ = function(req) {
    return req.i18n.__.bind(req.i18n);
};

I was rather asking why this original code wouldn't work. (The error I was getting was i18n in req being undefined, as if this function above was being evaluated before initializing i18n or something…)

I apologize if I'm not clear enough or if it's obvious I'm missing the knowledge to properly understand this.

maxwellium commented 11 years ago

im using express 3.1.0; patch doesn't seem to fix this for me. I am getting

Cannot read property '__' of undefined
  at Object.i18n.registerMethods.i18n.resMethods.forEach.helpers.(anonymous function) [as __] 
  (node_modules/i18n-2/i18n.js:97:19)

when I use

i18n.expressBind(app, {
  locales: ['de', 'en']
});

However, when i instead do

app.use(function(req, res, next) {

  req.i18n = new i18n({
    locales: ['de', 'en'],
    request: req
  });

  ['__','__n','getLocale','isPreferredLocale'].forEach(function(method){
    res.locals[method] = req.i18n[method].bind(req.i18n);
  });

  next();
});

directly, it works.

I have created a bare node project, using latest express and i18n-2, to eliminate possible interference. Am i doing sth wrong, or does this need fixing?

jeresig commented 11 years ago

@maxwellium that's really weird - where are you calling i18n.expressBind? It should be called outside of a .use() call. Looking at the code for i18n.registerMethods it appears to be doing exactly what you're doing so I'm not really sure what's up!

bitinn commented 11 years ago

hmm, I appear to face the same issue like @maxwellium, I put i18n.expressBind even before app.configure and still can't get it to work with swig.

Though I think most people are using swig through consolidate, think it would be an issue?

maxwellium commented 11 years ago

@jeresig I was on vacation for a bit, i did look into ur code and copy it pretty much 1to1 into my init block. i think, i was using your module before this change was checked into npm-registry. i'll try to replace "my" init procedure with your recommended function and report later today. but thx for looking into this!