bugfix(core): Allow circular structures for dynamic module #678

[BrunnerLivio]

PR Checklist

Please check if your PR fulfills the following requirements:

• The commit message follows our guidelines: https://github.com/nestjs/nest/blob/master/CONTRIBUTING.md
• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[x Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

What is the current behavior?

It does not support circular structures for dynamic modules

Issue Number: #678

What is the new behavior?

Supports circular structures for dynamic modules by replacing JSON.stringify with fast-safe-stringify which allows to stringify circular structures.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

[coveralls]

Coverage increased (+0.002%) to 93.709% when pulling 958da96 on BrunnerLivio:fix/circular-structures into 1acae78 on nestjs:master.

[BrunnerLivio]

If this PR gets approved we should consider adding e2e test for this edge case. It was quite annoying to migrate my Nest 4 app to Nest 5. Luckily I am a member of this framework, so I directly knew where to look, but a normal user would have been lost.

[kamilmysliwiec]

Thanks @BrunnerLivio, great job! Could you create an integration test in the integration/injector directory (based on the existing examples) :)?

[BrunnerLivio]

@kamilmysliwiec
Hmm can not run the integration tests. Looking through the travis.yml I even realized we do not run these in the pipeline haha.

I also had to install all the packages from the integration/* directories. I used

for D in integration/*; do [ -d "${D}" ] && npm i; done

Is this the intended way?

After I installed the dependencies and run npm run integration-test I became the following error:

TSError: ⨯ Unable to compile TypeScript
integration/graphql/e2e/graphql.spec.ts (14,5): Type 'INestApplication & INestExpressApplication' is not assignable to type 'INestApplication'.
  Types of property 'useWebSocketAdapter' are incompatible.
    Type '(adapter: WebSocketAdapter<any>) => INestApplication & INestExpressApplication' is not assignable to type '(adapter: WebSocketAdapter<any>) => INestApplication'.
      Types of parameters 'adapter' and 'adapter' are incompatible.
        Type 'WebSocketAdapter<any>' is not assignable to type 'WebSocketAdapter<any>'. Two different types with this name exist, but they are unrelated.
          Types of property 'bindMessageHandlers' are incompatible.
            Type '(client: any, handlers: { message: any; callback: (...args: any[]) => any; }[], transform: (data:...' is not assignable to type '(client: any, handlers: { message: any; callback: (...args: any[]) => any; }[], transform: (data:...'. Two different types with this name exist, but they are unrelated.
              Types of parameters 'transform' and 'transform' are incompatible.
                Type 'Observable<any>' is not assignable to type 'Observable<any>'. Two different types with this name exist, but they are unrelated.
                  Types of property 'source' are incompatible.
                    Type 'Observable<any>' is not assignable to type 'Observable<any>'. Two different types with this name exist, but they are unrelated. (2322)

Are these tests even up to date?

I think we really need a proper Contribution guide haha

[kamilmysliwiec]

We've got ~80 integration tests, they all are up to date (running on each publish) :) Simply call npm run build to move fresh packages to the integration directory (and yes, contribution guide is very required :D). Also, you can run docker-compose up -d inside integration dir to mount all necessary containers.

[BrunnerLivio]

@kamilmysliwiec done

Still did not got the graphql/mongoose test running because of the compile error, but I disabled them locally so I can work.

[BrunnerLivio]

@kamilmysliwiec can this be part of the 5.4.0 release? I saw this PR is not assigned to the 5.4.0. On purpose or not yet?

My private project release is currently blocked because of this bug, so I'd warmly welcome this in the next release..

[kamilmysliwiec]

@BrunnerLivio it's gonna be 5.3.11 which will be published in less than 1h 😅

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.