fix(core): Let InitializerService hook OnApplicationBootstrap instead of OnModuleInit #3638
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe updates shift lifecycle hook implementations between Changes
Sequence Diagram(s)sequenceDiagram
participant NestJS
participant ConfigModule
participant InitializerService
%% Module initialization phase
NestJS->>ConfigModule: onModuleInit()
ConfigModule->>ConfigModule: Initialize strategies and operations
%% Application bootstrap phase
NestJS->>InitializerService: onApplicationBootstrap()
InitializerService->>InitializerService: Await DB schema, initialize services, publish event
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/core/src/service/initializer.service.ts (1)
45-45: Consider updating the comment reference.The comment still references "onModuleInit" but the context is about lifecycle hooks in general, so the meaning is still clear. However, you may want to update it for consistency.
- // Nest's "onModuleInit" lifecycle hook within each individual service class? + // Nest's lifecycle hooks within each individual service class?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/core/src/config/config.module.ts(2 hunks)packages/core/src/service/initializer.service.ts(3 hunks)
🔇 Additional comments (6)
packages/core/src/config/config.module.ts (3)
1-1: LGTM: Import updated correctly for lifecycle hook change.The import change from
OnApplicationBootstraptoOnModuleInitis consistent with the lifecycle hook swap.
15-15: LGTM: Class interface updated correctly.The class now implements
OnModuleInitinstead ofOnApplicationBootstrap, which moves the initialization earlier in the lifecycle as intended.
21-24: LGTM: Method renamed and timing adjusted correctly.The method is now
onModuleInit()instead ofonApplicationBootstrap(), which ensures injectable strategies are initialized earlier in the lifecycle. This addresses the core issue where strategies need to be available beforeInitializerServiceruns.packages/core/src/service/initializer.service.ts (3)
1-1: LGTM: Import updated correctly for lifecycle hook change.The import change from
OnModuleInittoOnApplicationBootstrapis consistent with the lifecycle hook swap.
27-27: LGTM: Class interface updated correctly.The class now implements
OnApplicationBootstrapinstead ofOnModuleInit, which moves the initialization later in the lifecycle as intended. This ensures injectable strategies are available before this service runs.
42-60: LGTM: Method renamed and timing adjusted correctly.The method is now
onApplicationBootstrap()instead ofonModuleInit(), which ensures this service runs after theConfigModulehas initialized injectable strategies. This fixes the core timing issue described in the PR objectives.
|
Hi Daniel, thanks for taking some time to look into this. This is indeed quite a consequential change, and I'm also a bit nervous about the implications on people's plugins that might implicitly rely on the current order of execution. I'm not yet sure how best to handle this if it turns out we need to change it. |
While working on something else I found this logic bug, copying over from here so that this PR stays self contained:
In #3222 I am trying to introduce a new strategy which is needed during the intitialization phase of Vendure but due to the order of nest module life cycle hooks, Vendure currently fails to get injectables during initialization.
private getInjectableStrategies()- This is important because the strategies get initialized in the aboveinitInjectableStrategies()which gets called in the config modulesonApplicationBootstraphook. This is correct, but keyword being this specific lifecycle hook.InitializerServicewhich is responsible for calling Vendure-essential initializations sequentially, hooks intoonModuleInitwhich runs BEFOREonApplicationBootstrap-hooks i.e. before the config module.InitializerServicestarts initializing multiple things like channels, roles, admins, etc. BEFORE the config module was able to callinitInjectableStrategies()- which only gets called after, at application bootstrap.init(injector: Injector)function called in time, making essential dependencies likeTransactionalConnectionand other services undefined.I quickly skimmed through all the initializer functions and this bug has been hiding until today because none of the essential services used injectable strategies in their initialization. As soon as Vendure introduces some strategy, thats needed when initializing, this bug will occur.
Description
For testing purposes I swapped the hooks so that config module injects before initializerservice runs.
Breaking changes
Since this touches the very core of Vendure we must check thoroughly - are there further things to consider? I think nest calls the hooks concurrently through promise.all so a good solution must make sure the strategies and operations are always initialized before initialization-service runs, hence me swapping their hooks.
initInjectableStrategies()must run after all the plugins had the chance to alter the config.Checklist
📌 Always:
👍 Most of the time:
Summary by CodeRabbit