RFC: Split up health indicators into multiple packages

[BrunnerLivio]

The goal of this RFC is to validate the design with the community, solicit feedback on open questions, and enable experimentation via a non-production-ready prototype included in this proposal.

✏️ Motivation

@nestjs/terminus currently loads third party packages lazily. For instance, in order to use theTypeOrmHealthIndicator one must install @nestjs/typeorm and typeorm package.

Here is an example of the TypeOrmHealthIndicator implementing the package check:

terminus/lib/health-indicator/database/typeorm.health.ts

Lines 54 to 56 in 0dfc725

	private checkDependantPackages() {
	checkPackages(['@nestjs/typeorm', 'typeorm'], this.constructor.name);
	}

In order to do this, @nestjs/terminus check whether these packages can be loaded with the CommonJS require function

terminus/lib/utils/checkPackage.util.ts

Lines 32 to 40 in 0dfc725

	function optional<T = any>(module: string): T | null {
	try {
	if (module[0] in { '.': 1 }) {
	module = process.cwd() + module.substr(1);
	}
	return require(`${module}`);
	} catch (err) {}
	return null;
	}

This results in multiple pitfalls:

• ESBuild, Webpack, Yarn PNP does not work with optionally loading packages @nestjs/terminus cannot be embedded in a webpack bundle #1423
• ESM will be difficult / impossible to support
• Health Indicator implementations tend to be more complicated than necessary within @nestjs/terminus
  • In order to ensure that no TypeScript build errors occur, only types of the third party library can be referenced internally. This will make sure that @nestjs/typeorm is never referenced in the *.d.ts files. Since not everyone uses the TypeOrmHealthIndicator we always have to assume that this package does not exist. Because of this, third party types can also not be exported.

terminus/lib/health-indicator/database/typeorm.health.ts

Line 6 in 0dfc725

import * as NestJSTypeOrm from '@nestjs/typeorm';

terminus/lib/health-indicator/database/typeorm.health.ts

Lines 62 to 64 in 0dfc725

	const { getConnectionToken } =
	// eslint-disable-next-line @typescript-eslint/no-var-requires
	require('@nestjs/typeorm/dist/common/typeorm.utils') as typeof NestJSTypeOrm;

🔎 Implementation details

This RFC would try to minimally change implementations of the Health Indicators. The biggest change would be refactoring the different Health Indicators which require third-party dependencies.
The following Health Indicators would need to be extracted to its own package:

• TypeOrmHealthIndicator -> @nestjs/terminus-typeorm
• SequelizeHealthIndicator -> @nestjs/terminus-sequelize
• MongooseHealthIndicator -> @nestjs/terminus-mongoose
• MikroOrmHealthIndicator -> @nestjs/terminus-mikroorm
• HttpHealthIndicator -> @nestjs/terminus-http
• MicroserviceHealthIndicator -> @nestjs/terminus-microservices
• GRPCHealthIndicator -> @nestjs/terminus-grpc

The other Indicators could remain in the @nestjs/terminus package. @nestjs/terminus would still hold the logic to execute Health Indicators as well as providing common functionality to write Health Indicators.

⬆️ Main benefits of this proposal

• Support for @nestjs/terminus cannot be embedded in a webpack bundle #1423
• ESM support
• Simpler implementations of Health Indicators within @nestjs/terminus

⬇️ Main downsides of this proposal

• More dependencies to install. For instance, in order to use the NestJS TypeOrm integration with Terminus, one would need to install the following packages: typeorm @nestjs/typeorm @nestjs/terminus @nestjs/terminus-typeorm.

💡 Mitigation: Consider adding nest add @nestjs/terminus-schematic where one could select the health indicator needed for the project. This schematic could possibly auto-select health indicator by parsing through the package.json. So if the @nestjs/typeorm is already installed, most likely the user would want to have a health check with that library.

• Hefty breaking changes

❓ Mitigation: Open for ideas how to make the upgrade process simpler?

• Mono repository setup required which makes the build process & CI/CD more difficult.

[BrunnerLivio]

@Tony133 @kamilmysliwiec @micalevisk

Would love your input on this one.

[micalevisk]

👍

migrating the codebase would hurt a bit but I believe have dedicated packages will help on maintaining and improving @nestjs/terminus later on

[Tony133]

Hi @BrunnerLivio, i think it's an interesting idea to separate the health indicators into separate packages, it would make the @nestjs/terminus package easier to manage, it is normal that there are both advantages and disadvantages as you said.
But I also think that in terminus a similar is missing such as create a system as a dynamic module or [configurable module builder] (https://github.com/nestjs/docs.nestjs.com/pull/2357/files) (a new features in Nest v9 😻) which allow the developer to create their own health indicators with the use of third party librarys as sequelize, typeorm, etc. without overloading the work and maintenance times on your part by being part of the Core Team, it is normal that you cannot have 20 or more health indicators to maintain, the maintenance load would be unsustainable.At the moment I don't know if creating a "dynamic health indicator" is feasible, I have to explore a bit to see.
let's say at the moment that an "imaginative idea" 😄 💭

For example, thanks the on dynamic module guides and examples I found, I made 4 packages / modules for NestJS (knexjs, mongodbdriver, postgresql, mysql) , surely the developer must also be willing to develop and not having everything ready to use, I think this is the real challenge 😁

I hope I have given you some ideas and thanks for asking me for an opinion even if I am not part of the Core Team 😉👍

[BrunnerLivio]

@Tony133 Thanks for your input! I saw that feature before but just glossed over it :)
I don't see though how that in particular could solve the main issues I am trying to solve?

• Support for ESBuild, Webpack, Yarn PNP @nestjs/terminus cannot be embedded in a webpack bundle #1423
• ESM support

Maybe I missed something :)

[Tony133]

Hi @BrunnerLivio, for these two points, I'll answer you here below:

• Support for ESBuild, Webpack, Yarn PNP @nestjs/terminus cannot be embedded in a webpack bundle #1423

This point seems to me the very most complex, at the moment I have never done tests with terminus with webpack to study the behaviors and how to solve the problem. You have done some tests about it ?
Instead as regards ESBuild and PNP yarn are not yet "ready" for use (my opinion), they still need improvements and that resolve some incompatibilities, eg esbuild does not support typed declarations.

• ESM support

For ESM support I think you need to evaluate if all official NestJS modules will support it then yes, otherwise it's better not to think about it right away.

[BrunnerLivio]

This point seems to me the very most complex, at the moment I have never done tests with terminus with webpack to study the behaviors and how to solve the problem. You have done some tests about it ?

The problem most certainly lies in the code below. Essentially webpack can't predict whether Terminus is loading a package or not. Splitting the HealthIndicators into a package each would solve this issue because Terminus wouldn't need to load packages, the user would.

terminus/lib/utils/checkPackage.util.ts

Lines 32 to 40 in 0dfc725

	function optional<T = any>(module: string): T | null {
	try {
	if (module[0] in { '.': 1 }) {
	module = process.cwd() + module.substr(1);
	}
	return require(`${module}`);
	} catch (err) {}
	return null;
	}

For ESM support I think you need to evaluate if all official NestJS modules will support it then yes, otherwise it's better not to think about it right away.

I am aware the ESM is not supported yet. I don't think I could support ESM even if I wanted to at the moment due to third party packages. Nonetheless, it seems like ESM is coming no matter what. With this RFC I could support it when the time comes.

If there are other approaches to solve both of these issues I am very open for these suggestions. This RFC is just an idea to address these problems, though if I can avoid this additional complexity then I'd love to go with an alternative :)