search

i18next / i18next-express-middleware

[deprecated] can be replaced with i18next-http-middleware
https://github.com/i18next/i18next-http-middleware
MIT License
206 stars 52 forks source link

Update typings to allow req.i18n and req.t without Typescript error. #177

Closed nicolasigot closed 5 years ago

nicolasigot commented 5 years ago

Hello,

I missed these attributes in my previous PR. I hope I have all attributes now.

Thanks, Nicolas

jamuhl commented 5 years ago

published in i18next-express-middleware@1.7.3

isaachinman commented 4 years ago

@jamuhl Just a heads up - ran into a collision with next-i18next due to these typings. In my opinion, packages shouldn't be modifying global scope like this.

jamuhl commented 4 years ago

@isaachinman @nicolasigot if both agree we could revert this change?

isaachinman commented 4 years ago

For some context, I believe that exporting something like this is the most flexible way for a package to support typings - let your users consume the typings however they'd like.

jamuhl commented 4 years ago

@nicolasigot would you be able to provide a PR for suggested change?

nicolasigot commented 4 years ago

Hello @jamuhl , @isaachinman ,

Thanks for your feedback. I reviewed this case and the way it is exported in next-i18next will force the developper to cast the request object to a specific type each time he wants to access to specific properties or functions.

The usual way to do it by extending the express Request object in global is to benefit from typescript declaration merging. So that with the Request type we can access to all extended features of it. (https://www.typescriptlang.org/docs/handbook/declaration-merging.html)

I also checked how it is done for other express middleware and I found it the same way (ex: Passport) https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/passport/index.d.ts

I do not understand the conflict you have with next-i18next as you should extend it globally as well and from what I see you are adding options in i18n spec and this should be fine to do it globally as well. This is how you were doing before your commit (https://github.com/isaachinman/next-i18next/commit/294ab6f6a6d41f532ed797fd91a13e2a5614c75f#diff-d96ec7ae5bb4a0907779f25ed03acb33). Developers do not expect to use NextI18NextRequest objects but express.Request.

So I am not in favor of rolling back this update. I let you decide how you want to manage your typings.

Thanks, Nicolas

isaachinman commented 4 years ago

Hi @nicolasigot, thanks for your response. Let me explain a bit further.

Middlewares should not modify the global Express.Request interface, because the source code itself does not modify requests on a global level. It is a middleware.

Imagine, for a moment, a project wherein some routers use i18next-express-middleware and others don't. Modification at the global level would cause TypeScript to report that all req objects have things like req.i18n, when that is factually incorrect.

developper to cast the request object to a specific type each time he wants to access to specific properties or functions

Exactly right! That's to be expected, and is how TypeScript works.

isaachinman commented 4 years ago

Ah, and if you check the passport typings, you'll find a number of issues relating to typing conflicts with other packages, eg express-jwt and so on. Modifying the global scope is both incorrect and unmaintainable.

jamuhl commented 4 years ago

so we got an agreement that modifying global scope is nice but leads to even more problems beside all the problems typescript already adds ;)