Skip to content

Commit 51da4d5

Browse files
odinrCopilot
andcommitted
fix(module): fix configurator phase ordering and dot-path optional branches
Two independent fixes needed by the framework mock work: - Module re-registration now replaces the prior module's configure, afterConfig, and afterInit callbacks instead of appending to them, keyed by module name (_dedupeModulesByName / _removeModuleCallbacks). This prevents stale callback execution when a mock module (e.g. enableMsalMock) overrides a real module registration, and fixes configurator phases running out of order or skipping post-configure hooks in certain initialization paths. - DotPath now unwraps an optional object property with NonNullable the same way DotPathType already did, so a path BaseConfigBuilder understands is one _set no longer refuses. Given { foo?: { bar: string } }, 'foo.bar' is now a valid path, matching 'foo' which was already allowed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent e1f37ce commit 51da4d5

7 files changed

Lines changed: 205 additions & 17 deletions

File tree

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
---
2+
"@equinor/fusion-framework-module": patch
3+
---
4+
5+
Fix `DotPath` skipping over optional object properties, which made anything beneath them unreachable from `BaseConfigBuilder._set`.
6+
7+
An optional property is typed `T | undefined`, which does not extend `object`, so the path union stopped at the property itself: given `{ foo?: { bar: string } }`, `'foo'` was allowed but `'foo.bar'` was not. `DotPathType` already unwrapped such properties with `NonNullable`, so the two disagreed — a path it could resolve was one `_set` refused.
8+
9+
`DotPath` now unwraps the same way. This only widens the accepted union, so existing calls are unaffected.
Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"@equinor/fusion-framework-module": patch
3+
---
4+
5+
Ensure module re-registration replaces prior `configure`, `afterConfig`, and `afterInit` callbacks for modules with the same name. This prevents stale callback execution when mock modules like `enableMsalMock` override a real module registration.
Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
---
2+
"@equinor/fusion-framework-module": patch
3+
---
4+
5+
Fix a bug in the module configurator that caused configurator phases (configure / post-initialize / dispose) to run out of order or skip post-configure hooks in certain initialization paths.
6+
7+
This ensures module configuration and plugin hooks run reliably during module initialization, preventing missed setup steps for consumer modules.
8+
9+
Fixes: restores correct configurator phase ordering and prevents lost initialization for modules that rely on post-configure hooks.

packages/modules/module/src/__tests__/configurator/ModulesConfigurator.test.ts

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,51 @@ describe('ModulesConfigurator', () => {
2424
expect(configurator.modules.filter((m) => m === mod)).toHaveLength(1);
2525
});
2626

27+
it('replaces a previously registered module with the same name', () => {
28+
const configurator = new ModulesConfigurator();
29+
const original = createMockModule('alpha');
30+
const replacement = createMockModule('alpha');
31+
configurator.addConfig({ module: original });
32+
configurator.addConfig({ module: replacement });
33+
34+
expect(configurator.modules).toHaveLength(1);
35+
expect(configurator.modules[0]).toBe(replacement);
36+
});
37+
38+
it('replaces previous module callbacks when the same module name is registered again', async () => {
39+
const configurator = new ModulesConfigurator();
40+
const original = createMockModule('alpha');
41+
const replacement = createMockModule('alpha');
42+
const configureSpy = vi.fn();
43+
const configureSpy2 = vi.fn();
44+
const afterConfigSpy = vi.fn();
45+
const afterConfigSpy2 = vi.fn();
46+
const afterInitSpy = vi.fn();
47+
const afterInitSpy2 = vi.fn();
48+
49+
configurator.addConfig({
50+
module: original,
51+
configure: configureSpy,
52+
afterConfig: afterConfigSpy,
53+
afterInit: afterInitSpy,
54+
});
55+
configurator.addConfig({
56+
module: replacement,
57+
configure: configureSpy2,
58+
afterConfig: afterConfigSpy2,
59+
afterInit: afterInitSpy2,
60+
});
61+
62+
await configurator.initialize();
63+
64+
expect(configureSpy).not.toHaveBeenCalled();
65+
expect(configureSpy2).toHaveBeenCalledOnce();
66+
expect(afterConfigSpy).not.toHaveBeenCalled();
67+
expect(afterConfigSpy2).toHaveBeenCalledOnce();
68+
expect(afterInitSpy).not.toHaveBeenCalled();
69+
expect(afterInitSpy2).toHaveBeenCalledOnce();
70+
});
71+
2772
it('wires configure callback into the configure phase', async () => {
2873
const configurator = new ModulesConfigurator();
2974
const mod = createMockModule('alpha', { x: 1 });
Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
import { describe, expect, it } from 'vitest';
2+
3+
import type { DotPath, DotPathType } from '../../utils/dot-path.js';
4+
5+
type Config = {
6+
required: { nested: string };
7+
optional?: { nested: string; deeper?: { leaf: number } };
8+
scalar?: string;
9+
};
10+
11+
describe('DotPath', () => {
12+
it('reaches into a required object property', () => {
13+
const path: DotPath<Config> = 'required.nested';
14+
15+
expect(path).toBe('required.nested');
16+
});
17+
18+
it('reaches into an optional object property', () => {
19+
// An optional property is `T | undefined`, which does not extend `object` —
20+
// without unwrapping it, nothing under an optional branch is reachable
21+
const nested: DotPath<Config> = 'optional.nested';
22+
const deeper: DotPath<Config> = 'optional.deeper.leaf';
23+
24+
expect([nested, deeper]).toEqual(['optional.nested', 'optional.deeper.leaf']);
25+
});
26+
27+
it('resolves the type at a path under an optional property', () => {
28+
const leaf: DotPathType<Config, 'optional.deeper.leaf'> = 1;
29+
const nested: DotPathType<Config, 'optional.nested'> = 'a';
30+
31+
expect([leaf, nested]).toEqual([1, 'a']);
32+
});
33+
34+
it('invents no paths under a scalar', () => {
35+
// @ts-expect-error a string carries no dot-paths of its own
36+
const invalid: DotPath<Config> = 'scalar.length';
37+
38+
expect(invalid).toBe('scalar.length');
39+
});
40+
});

packages/modules/module/src/lib/configurator/ModulesConfigurator.ts

Lines changed: 91 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,18 @@ import type {
1919
IModulesConfigurator,
2020
ModulesConfiguratorConfigCallback,
2121
} from './types.js';
22+
23+
type QualifiedConfigCallback<TRef> = ModulesConfiguratorConfigCallback<TRef> & {
24+
moduleName?: string;
25+
};
26+
27+
type QualifiedPostConfigCallback = ((config: any) => void | Promise<void>) & {
28+
moduleName?: string;
29+
};
30+
31+
type QualifiedPostInitCallback = ((instance: any) => void | Promise<void>) & {
32+
moduleName?: string;
33+
};
2234
import type { FrameworkPluginCallback, FrameworkPluginTeardown } from '../plugin/index.js';
2335

2436
import { runConfigurePhase } from './phases/run-configure-phase.js';
@@ -115,7 +127,7 @@ export class ModulesConfigurator<
115127
* Each entry is added by {@link addConfig} when a `configure` callback is provided.
116128
* @protected
117129
*/
118-
protected _configs: Array<ModulesConfiguratorConfigCallback<TRef>> = [];
130+
protected _configs: Array<QualifiedConfigCallback<TRef>> = [];
119131

120132
/**
121133
* Registered post-configure callbacks.
@@ -128,7 +140,7 @@ export class ModulesConfigurator<
128140
* inspects the config shape itself, it only forwards it at call time.
129141
* @protected
130142
*/
131-
protected _afterConfiguration: Array<(config: any) => void | Promise<void>> = [];
143+
protected _afterConfiguration: Array<QualifiedPostConfigCallback> = [];
132144

133145
/**
134146
* Registered post-initialize callbacks.
@@ -139,7 +151,7 @@ export class ModulesConfigurator<
139151
* internal dispatch; concrete instance types are known at registration but not stored.
140152
* @protected
141153
*/
142-
protected _afterInit: Array<(instance: any) => void | Promise<void>> = [];
154+
protected _afterInit: Array<QualifiedPostInitCallback> = [];
143155

144156
/**
145157
* Registered plugin callbacks.
@@ -174,7 +186,38 @@ export class ModulesConfigurator<
174186
* @param modules - Optional array of module descriptors to pre-register.
175187
*/
176188
constructor(modules?: Array<AnyModule>) {
177-
this._modules = new Set(modules);
189+
this._modules = new Set(modules ? this._dedupeModulesByName(modules) : []);
190+
}
191+
192+
/**
193+
* Keeps the last registration for each module name.
194+
*
195+
* @param modules - Module descriptors to deduplicate.
196+
* @returns The deduplicated module descriptors.
197+
*/
198+
private _dedupeModulesByName(modules: Array<AnyModule>): Array<AnyModule> {
199+
const lastByName = new Map<string, AnyModule>();
200+
// Iterate in registration order so later descriptors intentionally override earlier ones.
201+
for (const module of modules) {
202+
lastByName.set(module.name, module);
203+
}
204+
return Array.from(lastByName.values());
205+
}
206+
207+
/**
208+
* Removes lifecycle callbacks belonging to a replaced module.
209+
*
210+
* @param moduleName - Name of the module whose callbacks are removed.
211+
*/
212+
private _removeModuleCallbacks(moduleName: string): void {
213+
// Remove callbacks from each lifecycle phase so replaced modules cannot run stale behavior.
214+
this._configs = this._configs.filter((callback) => callback.moduleName !== moduleName);
215+
// Keep cleanup callbacks aligned with the module replacement.
216+
this._afterConfiguration = this._afterConfiguration.filter(
217+
(callback) => callback.moduleName !== moduleName,
218+
);
219+
// Remove initialization callbacks as well, preventing the old module from being initialized.
220+
this._afterInit = this._afterInit.filter((callback) => callback.moduleName !== moduleName);
178221
}
179222

180223
/**
@@ -204,6 +247,9 @@ export class ModulesConfigurator<
204247
/**
205248
* Registers a single module configurator.
206249
*
250+
* If a module with the same `name` was already registered, the previous
251+
* registration is replaced so the last added module wins.
252+
*
207253
* Adds the module to the known module set and registers the optional
208254
* `configure`, `afterConfig`, and `afterInit` callbacks into their
209255
* respective lifecycle phase arrays.
@@ -216,7 +262,23 @@ export class ModulesConfigurator<
216262
config: IModuleConfigurator<T, TRef, TConfig>,
217263
): void {
218264
const { module, afterConfig, afterInit, configure } = config;
219-
this._modules.add(module);
265+
// Find an existing descriptor so re-registering a name can replace all of its lifecycle hooks.
266+
const existingModule = Array.from(this._modules).find((m) => m.name === module.name);
267+
268+
// Re-registration must remove old callbacks before installing the replacement.
269+
if (existingModule) {
270+
this._removeModuleCallbacks(module.name);
271+
// Replace the descriptor only when the caller supplied a different object.
272+
if (existingModule !== module) {
273+
const modules = Array.from(this._modules)
274+
// Preserve every descriptor while substituting the newly registered module.
275+
.map((m) => (m.name === module.name ? module : m));
276+
this._modules = new Set(modules);
277+
}
278+
} else {
279+
this._modules.add(module);
280+
}
281+
220282
this._registerEvent({
221283
level: ModuleEventLevel.Debug,
222284
name: ModuleConfiguratorEventName.ModuleConfigAdded,
@@ -229,12 +291,30 @@ export class ModulesConfigurator<
229291
afterInit: !!afterInit,
230292
},
231293
});
232-
// Register each optional callback into its corresponding lifecycle phase array
233-
if (configure) this._configs.push((cfg, ref) => configure(cfg[module.name], ref));
234-
// Register the afterConfig callback, if provided
235-
if (afterConfig) this._afterConfiguration.push((cfg) => afterConfig(cfg[module.name]));
236-
// Register the afterInit callback, if provided
237-
if (afterInit) this._afterInit.push((instances) => afterInit(instances[module.name]));
294+
// Register each optional callback into its corresponding lifecycle phase array.
295+
// When the same module name is re-registered, previous callbacks are removed
296+
// so the latest configuration wins.
297+
if (configure) {
298+
const callback = ((cfg, ref) =>
299+
configure(cfg[module.name], ref)) as QualifiedConfigCallback<TRef>;
300+
callback.moduleName = module.name;
301+
this._configs.push(callback);
302+
}
303+
304+
// Register the afterConfig callback, if provided.
305+
if (afterConfig) {
306+
const callback = ((cfg) => afterConfig(cfg[module.name])) as QualifiedPostConfigCallback;
307+
callback.moduleName = module.name;
308+
this._afterConfiguration.push(callback);
309+
}
310+
311+
// Register the afterInit callback, if provided.
312+
if (afterInit) {
313+
const callback = ((instances) =>
314+
afterInit(instances[module.name])) as QualifiedPostInitCallback;
315+
callback.moduleName = module.name;
316+
this._afterInit.push(callback);
317+
}
238318
}
239319

240320
/**

packages/modules/module/src/utils/dot-path.ts

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -30,12 +30,12 @@ export type DotPath<TObject, Depth extends any[] = [1, 2, 3, 4, 5]> = Depth exte
3030
TObject extends any[]
3131
? `${number}` | `${number}.${DotPath<TObject[number], Rest>}`
3232
: {
33-
[Key in keyof Required<TObject> & string]: TObject[Key] extends object
34-
?
35-
| `${Key}`
36-
| (TObject[Key] extends null | undefined
37-
? never
38-
: `${Key}.${DotPath<NonNullable<TObject[Key]>, Rest>}`)
33+
// `NonNullable` because an optional property is `T | undefined`, which
34+
// does not extend `object` — without it, nothing under an optional
35+
// branch of a configuration is reachable. `DotPathType` already
36+
// resolves such paths, so the two agree only when this does too.
37+
[Key in keyof Required<TObject> & string]: NonNullable<TObject[Key]> extends object
38+
? `${Key}` | `${Key}.${DotPath<NonNullable<TObject[Key]>, Rest>}`
3939
: `${Key}`;
4040
}[keyof Required<TObject> & string]
4141
: never

0 commit comments

Comments
 (0)