Skip to content

Conversation

@lucas-gregoire
Copy link
Contributor

@lucas-gregoire lucas-gregoire commented Nov 3, 2023

Description

I discovered a bug when using disableMiddleware: true.
When relying on I18nLanguageInterceptor to initialize the I18nContext, it's not possible to use another interceptor on the same endpoint.
When doing so, the AsyncLocalStorage containing the i18n context is broken: I18nContext.current() always returns undefined.

How to reproduce

  • Set disableMiddleware: true in I18nModule options.
  • Define a simple interceptor such as:
@Injectable()
export class TestInterceptor implements NestInterceptor {
  intercept(_: ExecutionContext, next: CallHandler): Observable<unknown> {
    return next.handle();
  }
}
  • Add it on your endpoint:
@Get()
@UseInterceptors(TestInterceptor)
helloWorld() {
  return I18nContext.current()?.lang;
}
  • This endpoint will always return undefined instead of the resolved lang. When removing the interceptor it works.

Additional context

I did something similar to the nestjs-cls's interceptor implementation: https://github.com/Papooch/nestjs-cls/blob/main/packages/core/src/lib/cls-initializers/cls.interceptor.ts
I just simplified it by passing the observer directly to the subscribe method.
I added a test in i18n-disable-middleware.e2e.spec.ts to avoid regressions.

See also:

@lucas-gregoire lucas-gregoire changed the title fix: I18nContext.current() undefined with async interceptors fix: I18nContext.current() undefined when using additional interceptors Nov 3, 2023
@coveralls
Copy link

coveralls commented Nov 3, 2023

Coverage Status

coverage: 90.869% (+0.1%) from 90.764%
when pulling 45d148d on lucas-gregoire:fix/undefined-current-context
into 0774e23 on toonvanstrijp:main.

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.

3 participants