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

[Bug]: Expected OgmaCoreModule to be configured by at last one Module but it was not configured within 0ms #228

Closed
Ponjimon opened this issue Sep 15, 2020 · 10 comments

Comments

@Ponjimon
Copy link

Bug Report

I get the above error when running ^0.2.2 and above but not with ^0.1.0

I cannot find any breaking change about this?

Current behavior

The app doesn't start because of that error.

Expected behavior

It should start.

Environment


@ogma/nestjs-module version: ^0.2.2 and above
 
For Tooling issues:
- Node version: v12.16.1
- Platform:  Ubuntu 18.04 (WSL 2)

Others:

@jmcdo29
Copy link
Owner

jmcdo29 commented Sep 15, 2020

Can you provide a minimum reproduction?

@jjaracz
Copy link

jjaracz commented Sep 20, 2020

@jmcdo29 it's related to register .forFeature and in the same module .forRoot.
Try to move it to separate modules and import it to one common, it's only workaround, we need to address that issue.

Another way to make it work is workaround .forFeature and inject provider by yourself (eg. I solve that way on tests because there I didn't have a good way to separate it to modules), but then if you would like to inject it to your module imo it will be a very bad decision because you would need to use instance of OgmaService where you need to provide options etc. so use core features of library, for tests it works because there you don't need to provide instance just mock (as far as you wouldn't need logs in your tests).

{
  ...
  providers: [
    {
      provide: createRequestScopedProviderToken(SomeService.name),
      useValue: jest.fn()
    }
  ]
}

(It's for request scoped providers, if you would like to use not request scoped use createProviderToken instead)

@jjaracz
Copy link

jjaracz commented Sep 20, 2020

It's probably related to dependency @golevelup/nestjs-modules and not in @ogma itself

@Ponjimon
Copy link
Author

I did that now, the error is gone, but there is no log at all. I added some console logs to ogma.interceptor.js inside node_modules and OgmaInterceptor is not even being instanciated...

@jmcdo29
Copy link
Owner

jmcdo29 commented Sep 24, 2020

In version 0.3.0 and on, the interceptor is no longer auto attached. This is to allow for better customization of the request id generation.

From the README for @ogma/nestjs-module

Note: As of version 0.3.0 the OgmaInterceptor is not bound by default, it is still necessary to pass all of the expected configuration options due to the way that the dependencies are built. If you would like to bind the interceptor globally, you can still do so using APP_INTERCEPTOR in a custom provider. The interceptor is not bound by default anymore to allow for more customization when it comes to the generation of request IDs.

@Ponjimon
Copy link
Author

So, the interceptor works now, but the main issue is still there.

I also made a codesandbox where you can reproduce it.

On the left navigation bar, go to the Server Control Panel and click Restart Server. In the terminal output you will then see the above error message.

https://codesandbox.io/s/nest-typescript-starter-forked-ofwpb?file=/src/app.module.ts

I also included the LoggerModule I'm using in my project in this codesandbox, maybe I'm doing something wrong, but afaik, I'm just using it as per documentation so it should work?

It's logging in the AppService

@jmcdo29
Copy link
Owner

jmcdo29 commented Sep 29, 2020

@Ponjimon in the future, GitHub repositories are preferable to codesandbox reproductions.

I noticed in your package.json you had several @nestjs/ dependencies still on version five while others were up on version 7. These should always keep the same major version, just a heads up.

I was able to reproduce the bug, and have a workaround for the time being. I never really anticipated anyone to move the OgmaModule.forRoot/Async() method out of the RootModule, so this wasn't in my suite of tests. It's a problem with Nest's module resolution system and with how @golevelup/nestjs-modules works with creating dynamic modules. For now, what can be done, in your LoggerModule do something like this:

import { Module } from '@nestjs/common';
import { APP_FILTER, APP_INTERCEPTOR } from '@nestjs/core';
import { OgmaModule, OgmaModuleOptions } from '@ogma/nestjs-module';
import { ExpressParser } from '@ogma/platform-express';
import { GraphQLParser } from '@ogma/platform-graphql';
import { AppService } from 'app.service';
import { GqlHttpExceptionFilter } from './gql-http-exception.filter';

import { LoggingInterceptor } from './logging.interceptor';

export const OGMA_MODULE_OPTIONS: OgmaModuleOptions = {
  service: {
    color: Boolean(process.env.IS_OFFLINE),
  },
  interceptor: {
    http: ExpressParser,
    gql: GraphQLParser,
  },
};

@Module({
  imports: [
    OgmaModule.forFeature(AppService),
    OgmaModule.forRootAsync({
      useFactory: (): OgmaModuleOptions | Promise<OgmaModuleOptions> =>
        OGMA_MODULE_OPTIONS,
    }),
  ],
  providers: [
    { provide: APP_INTERCEPTOR, useClass: LoggingInterceptor },
    { provide: APP_FILTER, useClass: GqlHttpExceptionFilter },
  ],
  exports: [OgmaModule]
})
export class LoggerModule {}

Creating the OgmaModule.forFeature() in there and exporting it, then in your AppModule just having imports: [LoggerModule]

I'll see what I can do to make this a better dev experience. I'm thinking I may just have to write the dynamic module myself and not use the @golevelup package, but we'll see if there's anything else I can do first.

@Ponjimon
Copy link
Author

Ponjimon commented Sep 29, 2020

That's a bummer, having to do that inside the LoggerModule makes the whole purpose of that module obsolete because that creates a circular dependency ModuleThatUsesLogger <=> LoggerModule.

Thanks anyway, hopefully, this can be fixed in a future update.

The reason I did it that way was because I am using this LoggerModule in many Nest applications and not just one, so I wanted to reduce code (duplication).

@jmcdo29
Copy link
Owner

jmcdo29 commented Oct 23, 2020

To finally circle back around to this, I'm gonna work on phasing out the usage of OgmaCoreModule.Deferred and just use a @Global() decorator instead. I'll make sure to add this to the test suite and see if it fixes the issue there 😄 I'll try to have something up by the end of the weekend

jmcdo29 added a commit that referenced this issue Oct 25, 2020
The log methods now accept an object instead of multiple parameters to allow for the passing of
extra keys. These keys can be anything, from cloud-provider to anything else you want to add.
There's also a lot of refactoring and making some new interfaces for cleaner passing of parameters

BREAKING CHANGE: log methods now take an object as the second parameter instead of having 3 extra
optional parameters

fix #215 #228 #297
@jmcdo29 jmcdo29 mentioned this issue Oct 25, 2020
jmcdo29 added a commit that referenced this issue Oct 25, 2020
The log methods now accept an object instead of multiple parameters to allow for the passing of
extra keys. These keys can be anything, from cloud-provider to anything else you want to add.
There's also a lot of refactoring and making some new interfaces for cleaner passing of parameters

BREAKING CHANGE: log methods now take an object as the second parameter instead of having 3 extra
optional parameters

fix #215 #228 #297
@jmcdo29
Copy link
Owner

jmcdo29 commented Oct 25, 2020

Added to v0.4.0. This will most likely be the last pre v1 version. Thanks for the idea

@jmcdo29 jmcdo29 closed this as completed Oct 25, 2020
jmcdo29 added a commit that referenced this issue Jun 20, 2021
The log methods now accept an object instead of multiple parameters to allow for the passing of
extra keys. These keys can be anything, from cloud-provider to anything else you want to add.
There's also a lot of refactoring and making some new interfaces for cleaner passing of parameters

BREAKING CHANGE: log methods now take an object as the second parameter instead of having 3 extra
optional parameters

fix #215 #228 #297
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

No branches or pull requests

3 participants