fix(core): Order of module destroy should be the reverse of module init

[wenlong-chen]

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?

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

Suppose A depends on B, currently, A.onModuleDestroy is executed after B.onModuleDestroy.

Issue Number: #14118

What is the new behavior?

Suppose A depends on B, A.onModuleDestroy is executed before B.onModuleDestroy.

Does this PR introduce a breaking change?

• Yes
• No

Other information

[coveralls]

Pull Request Test Coverage Report for Build cbf75015-de20-4745-afe3-883731c322a1

Details

• 0 of 0 changed or added relevant lines in 0 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 92.243%

---

Totals
Change from base Build 72f148bc-4145-44b3-a3c4-06a616307114:	0.0%
Covered Lines:	6754
Relevant Lines:	7322

---

💛 - Coveralls

[kamilmysliwiec]

Let's consider the following example:

1. A depends on B (AModule -> BModule)
2. Before tearing down, A needs to perform some actions and it needs B (its dependency) to do this
3. This is currently feasible as the order reflects relations between providers (a->b), meaning, the outer gets executed first
4. If we merged this PR, this would no longer be doable as B would have been destroyed before A (meaning, when onModuleDestroy was called for A, A would no longer be able to interact with any of its dependencies coming from B)

In summary, although reversing the order might seem intuitive, it actually creates more issues than it resolves. Especially since B should never interact directly with A unless a circular relationship is introduced—in which case, the order wouldn’t matter anyway.

[wenlong-chen]

@kamilmysliwiec I completely agree with your view on the order of the onModuleDestroy sequence, and this PR aims to achieve that expected order as well. The current behavior does not align with the expected sequence.

Assumption: A depends on B.
Expected behavior: First A.onModuleDestroy, then B.onModuleDestroy.
Current behavior: First B.onModuleDestroy, then A.onModuleDestroy.

The current test incorrectly expects B.onModuleDestroy to execute before A.onModuleDestroy.

nest/integration/hooks/e2e/on-module-destroy.spec.ts

Lines 43 to 83 in bc4667c

	it('should sort modules by distance (topological sort) - DESC order', async () => {
	@Injectable()
	class BB implements OnModuleDestroy {
	public field: string;
	async onModuleDestroy() {
	this.field = 'b-field';
	}
	}
	@Module({
	providers: [BB],
	exports: [BB],
	})
	class B {}
	@Injectable()
	class AA implements OnModuleDestroy {
	public field: string;
	constructor(private bb: BB) {}
	async onModuleDestroy() {
	this.field = this.bb.field + '_a-field';
	}
	}
	@Module({
	imports: [B],
	providers: [AA],
	})
	class A {}
	const module = await Test.createTestingModule({
	imports: [A],
	}).compile();
	const app = module.createNestApplication();
	await app.init();
	await app.close();
	const instance = module.get(AA);
	expect(instance.field).to.equal('b-field_a-field');
	});

My PR expects A.onModuleDestroy to execute before B.onModuleDestroy.

nest/integration/hooks/e2e/on-module-destroy.spec.ts

Lines 43 to 78 in 34c2cca

	it('should sort modules by distance (topological sort) - DESC order', async () => {
	@Injectable()
	class BB implements OnModuleDestroy {
	onModuleDestroy = Sinon.spy();
	}
	@Module({
	providers: [BB],
	exports: [BB],
	})
	class B {}
	@Injectable()
	class AA implements OnModuleDestroy {
	constructor(private bb: BB) {}
	onModuleDestroy = Sinon.spy();
	}
	@Module({
	imports: [B],
	providers: [AA],
	})
	class A {}
	const module = await Test.createTestingModule({
	imports: [A],
	}).compile();
	const app = module.createNestApplication();
	await app.init();
	await app.close();
	const aa = module.get(AA);
	const bb = module.get(BB);
	Sinon.assert.callOrder(aa.onModuleDestroy, bb.onModuleDestroy);
	});

[wenlong-chen]

@kamilmysliwiec I’ve created a minimal reproduction code. Please take a look. https://github.com/wenlong-chen/nest-on-module-destory-bug

The following code throws an error during shutdown, which is unexpected.
https://github.com/wenlong-chen/nest-on-module-destory-bug/blob/54457903436aeae239e86441de43441ec8643138/src/app.ts#L4-L45