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

feat(health-indicator): add mikro-orm health indicator #1876

Closed
wants to merge 4 commits into from

Conversation

jbw
Copy link
Contributor

@jbw jbw commented Jun 24, 2022

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • 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 new behavior?

Adds mikro-orm health indicator

Issue Number: #1877

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@jbw jbw marked this pull request as ready for review June 24, 2022 14:22
@BrunnerLivio
Copy link
Member

LGTM so far. I'll check it out locally asap :) Thanks a lot, really appreicated!

@jbw
Copy link
Contributor Author

jbw commented Jun 24, 2022

LGTM so far. I'll check it out locally asap :) Thanks a lot, really appreicated!

The unit tests failed here but not sure why. Does work locally though 😉

@micalevisk
Copy link
Member

micalevisk commented Jun 25, 2022

@jbw they failed in node10 because the operator ?? was introduced in node14. Not sure how we could fix that as that code isn't in our side 🤔

@jbw
Copy link
Contributor Author

jbw commented Jun 25, 2022

@jbw they failed in node10 because the operator ?? was introduced in node14. Not sure how we could fix that as that code isn't in our side 🤔

Downgrading Mikro that far back to v2 or v3 won't work either as the library is not compatible with newer versions. v4 onwards supports Node 14+

@BrunnerLivio @micalevisk I noticed the e2e tests support only v14. I wonder if it is safe to drop v10 and v12 from the unit tests?

@micalevisk
Copy link
Member

micalevisk commented Jun 25, 2022

Kamil still want to support node 10. So I guess isn't that safe to remove those unit tests in @nestjs/* packages.

Maybe we can drop the support for node10 only for the mikro-orm part somehow. And then we should verify that in the action so we could skip it

@BrunnerLivio
Copy link
Member

BrunnerLivio commented Jun 26, 2022

I'm doing some further experiments on this. Theoretically, it should be possible to keep Node v10 support with Terminus - unless you use MikroOrm off course - since we optionally load third-party dependencies. I'll close this PR and continue it over at #1879. Off course, contributions are welcome there as well.

FYI:

  • If I am not mistaken I was able to run NestJS Terminus with Node v10 + the changes from this PR. I published @nestjs/terminus@beta with the changes from this PR to test it. Need to dig a bit deeper but so far it seems feasible.

  • I am currently struggling with getting the MikroORM provider without importing the MikroORM class itself. Terminus does this so in the type definition file there are no references to MikroORM so TypeScript does not complain in case the dependency is not installed.

I am trying it this way currently, no luck so far.

const { MikroORM } = require('@mikro-orm/nestjs') as typeof MikroOrmNestJS;
const mikro = this.moduleRef.get(MikroORM, { strict: false });

Will need to dig deeper in the following days

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