i18next / i18next-http-middleware

i18next-http-middleware is a middleware to be used with Node.js web frameworks like express or Fastify and also for Deno.
MIT License
153 stars 28 forks source link

res.locals can't be expected to be defined #15

Closed eric-burel closed 4 years ago

eric-burel commented 4 years ago

🐛 Bug Report

res.locals is not populated.

To Reproduce

I don't have a minimal reproduction. However this may happen in Next, using the new beta version of next-i18next (serverless version), that itself rely on this package.

For some unknown reason,  res.locals is set in development, but not in the build app. That makes sense anyway, because the Next server is not an Express server per se, so there is no reason for res.locals to be set in the first place.

If really needed I can easily provided a repro, just not really minimal.

Expected behavior

I'd be in favour of creating the res.locals object, when it is not defined, in https://github.com/i18next/i18next-express-middleware/blob/master/src/index.js.

This way we keep a consistent behavior in servers that don't initially define a "res.locals" object.

Your Environment

Next 9.4 with next-i18next v5 (beta)

eric-burel commented 4 years ago

See https://github.com/isaachinman/next-i18next/issues/20#issuecomment-645966942

adrai commented 4 years ago

Maybe I'm not seeing the issue, but this was with i18next-express-middleware: https://github.com/i18next/i18next-express-middleware/blob/master/src/index.js#L23 this is now with i18next-http-middleware: https://github.com/i18next/i18next-http-middleware/blob/master/lib/index.js#L43

     if (res.locals) {
        res.locals.language = lng
        res.locals.languageDir = i18next.dir(lng)
      }

There must be some difference in your code (or next, or wherever) between dev and prod...

eric-burel commented 4 years ago

The if (res.locals) in itself is problematic, because res.locals is an Express convention. This condition may be always false if you use another Node framework.

Agreed, there's something fishy in the fact that it works differently in both environment, but I think that it should not even work in the first place.

If you think it should stay as is in the code, the alternative is to add a documentation explaining that you expect res.locals to be set in order to get language and languageDir, and let the user define it manually depending on their own needs.

adrai commented 4 years ago

if res.locals is false, this is ok... I do NOT expect res.locals to be set... this is mainly used for stuff like this: https://github.com/i18next/i18next-http-middleware/tree/master/example/basic-pug https://github.com/i18next/i18next-http-middleware/tree/master/example/fastify-pug

adrai commented 4 years ago

btw: what do you mean by "in order to get language and languageDir"... this is only for templating stuff, i.e. for pug... Are you using language and languageDir from locals in next? you should have all you need on the request object: req.i18n => the i18next instance req.language etc...

adrai commented 4 years ago

Nevertheless, if you have some example repo, this would for sure help everyone to invetigate...

adrai commented 4 years ago

@isaachinman are you referring to locals somewhere in next-i18next?

isaachinman commented 4 years ago

@adrai Nope. This doesn't really have anything to do with next-i18next.

adrai commented 4 years ago

So seems only @eric-burel can tell us why he relies on locals 🤷‍♂️

eric-burel commented 4 years ago

Hi, indeed this is for injecting the language into the html tag, during server-side render. There is no official doc but this approach was recommended by users in comments of next-i18next repo, and as far as I understand it's the most effective way to get those info.

I am coding a Next starter, so I can repro. It's not very minimal however, sorry, but it demo the issue consistently. See https://github.com/VulcanJS/vulcan-next-starter/issues/43

adrai commented 4 years ago

Maybe something like this will work: image

eric-burel commented 4 years ago

Awesome, that fixes everything. I wasn't sure req would be filled correctly and I was mislead by examples I've found in the wild, relying on res.locals.

In TS my final code looks like this:

// We except a middleware to enhance the server response
interface IncomingMessageWithI18n extends IncomingMessage {
  language?: string;
  i18n: any;
}
export const i18nPropsFromCtx = (
  ctx: NextPageContext
  //req?: Inc
  //res?: ServerResponseWithLocals
): Partial<HtmlLanguageProps> => {
  if (!(ctx && ctx.req && (ctx.req as IncomingMessageWithI18n).language))
    return {};
  const req = ctx.req as IncomingMessageWithI18n;
  return {
    lang: req.language,
    dir: req.i18n && req.i18n.dir(req.language),
  };
};

Thanks for the fast and detailed answer.

adrai commented 4 years ago

If you like this module don’t forget to star this repo. Make a tweet, share the word or have a look at our https://locize.com to support the devs of this project.

There are many ways to help this project 🙏