Skip to content

Commit 30de256

Browse files
authored
fix(macros): follow up on member access review fixes (#1156)
* codex: address PR review feedback (#1155) * codex: address PR review feedback (#1156)
1 parent a111204 commit 30de256

File tree

3 files changed

+56
-45
lines changed

3 files changed

+56
-45
lines changed

src/engine/SingleMacroEngine.member-access.test.ts

Lines changed: 47 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -328,6 +328,52 @@ describe("SingleMacroEngine member access", () => {
328328
expect(mockGetUserScript).toHaveBeenCalledWith(scriptB, app);
329329
});
330330

331+
it("loads a selector-targeted script after pre-commands run", async () => {
332+
const preCommand = {
333+
id: "wait-1",
334+
name: "Wait",
335+
type: CommandType.Wait,
336+
} as ICommand;
337+
const scriptA = createUserScript("script-a", "a.js", {
338+
name: "Alpha Script",
339+
});
340+
const scriptB = createUserScript("script-b", "b.js", {
341+
name: "Beta Script",
342+
});
343+
344+
let ready = false;
345+
const engineInstance = macroEngineFactory();
346+
engineInstance.runSubset = vi.fn().mockImplementation(async () => {
347+
ready = true;
348+
});
349+
macroEngineFactory = () => engineInstance;
350+
351+
mockGetUserScript.mockImplementation(async (command: IUserScript) => {
352+
if (command.id !== "script-b") {
353+
return { alpha: vi.fn() };
354+
}
355+
356+
return ready
357+
? { beta: () => "late-bound-result" }
358+
: { alpha: vi.fn() };
359+
});
360+
361+
const engine = new SingleMacroEngine(
362+
app,
363+
plugin,
364+
[baseMacroChoice([preCommand, scriptA, scriptB])],
365+
choiceExecutor,
366+
);
367+
368+
const result = await engine.runAndGetOutput(
369+
"My Macro::Beta Script::beta",
370+
);
371+
372+
expect(result).toBe("late-bound-result");
373+
expect(mockGetUserScript).toHaveBeenCalledTimes(1);
374+
expect(mockGetUserScript).toHaveBeenCalledWith(scriptB, app);
375+
});
376+
331377
it("aborts when a selector matches duplicate script names", async () => {
332378
const scriptA = createUserScript("script-a", "a.js", {
333379
name: "Shared Script",
@@ -411,7 +457,7 @@ describe("SingleMacroEngine member access", () => {
411457

412458
await expect(
413459
engine.runAndGetOutput("My Macro::Alpha Script::beta"),
414-
).rejects.toThrow("targeted script 'Alpha Script'");
460+
).rejects.toThrow("routes member access to 'Alpha Script'");
415461
});
416462

417463
it("propagates aborts when the export aborts", async () => {

src/engine/SingleMacroEngine.ts

Lines changed: 7 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ type UserScriptCandidate = {
1717
command: IUserScript;
1818
index: number;
1919
exportsRef?: unknown;
20-
resolvedMember: { found: boolean; value?: unknown };
20+
resolvedMember?: { found: boolean; value?: unknown };
2121
};
2222

2323
type MemberAccessSelection = {
@@ -98,16 +98,14 @@ export class SingleMacroEngine {
9898
throw new Error(`macro '${macroName}' does not exist.`);
9999
}
100100

101-
const preloadedScripts = new Map<string, unknown>();
102-
103101
// Create a dedicated engine for this macro
104102
const engine = new MacroChoiceEngine(
105103
this.app,
106104
this.plugin,
107105
macroChoice,
108106
this.choiceExecutor,
109107
this.variables,
110-
preloadedScripts,
108+
undefined,
111109
context?.label,
112110
);
113111

@@ -116,7 +114,6 @@ export class SingleMacroEngine {
116114
engine,
117115
macroChoice,
118116
memberAccess,
119-
preloadedScripts,
120117
);
121118

122119
this.ensureNotAborted();
@@ -154,7 +151,6 @@ export class SingleMacroEngine {
154151
engine: MacroChoiceEngine,
155152
macroChoice: IMacroChoice,
156153
memberAccess: string[],
157-
preloadedScripts: Map<string, unknown>,
158154
): Promise<{ executed: boolean; result?: unknown }> {
159155
const originalCommands = macroChoice.macro?.commands;
160156
if (!originalCommands?.length) {
@@ -176,7 +172,6 @@ export class SingleMacroEngine {
176172
macroChoice,
177173
userScriptCommands,
178174
memberAccess,
179-
preloadedScripts,
180175
);
181176
const preCommands = originalCommands.slice(0, selection.candidate.index);
182177

@@ -210,11 +205,6 @@ export class SingleMacroEngine {
210205
);
211206
}
212207

213-
const cacheKey = userScriptCommand.path ?? userScriptCommand.id;
214-
if (cacheKey && exportsRef !== undefined && exportsRef !== null) {
215-
preloadedScripts.set(cacheKey, exportsRef);
216-
}
217-
218208
const settingsExport =
219209
typeof exportsRef === "object" || typeof exportsRef === "function"
220210
? (exportsRef as Record<string, unknown>).settings
@@ -228,9 +218,8 @@ export class SingleMacroEngine {
228218
}
229219

230220
const resolvedMember =
231-
selection.candidate.exportsRef !== undefined
232-
? selection.candidate.resolvedMember
233-
: this.resolveMemberAccess(exportsRef, selection.memberAccess);
221+
selection.candidate.resolvedMember ??
222+
this.resolveMemberAccess(exportsRef, selection.memberAccess);
234223

235224
if (!resolvedMember.found) {
236225
throw new MacroAbortError(
@@ -327,14 +316,12 @@ export class SingleMacroEngine {
327316
macroChoice: IMacroChoice,
328317
userScriptCommands: Array<{ command: IUserScript; index: number }>,
329318
memberAccess: string[],
330-
preloadedScripts: Map<string, unknown>,
331319
): Promise<MemberAccessSelection> {
332320
if (userScriptCommands.length === 1) {
333321
return {
334322
candidate: {
335323
command: userScriptCommands[0].command,
336324
index: userScriptCommands[0].index,
337-
resolvedMember: { found: false },
338325
},
339326
memberAccess,
340327
};
@@ -347,44 +334,21 @@ export class SingleMacroEngine {
347334
);
348335

349336
if (selectorMatch) {
350-
const exportsRef = await getUserScript(selectorMatch.command, this.app);
351-
const cacheKey = selectorMatch.command.path ?? selectorMatch.command.id;
352-
if (cacheKey && exportsRef !== undefined && exportsRef !== null) {
353-
preloadedScripts.set(cacheKey, exportsRef);
354-
}
355-
356-
const resolvedMember = this.resolveMemberAccess(
357-
exportsRef,
358-
selectorMatch.memberAccess,
359-
);
360-
if (!resolvedMember.found) {
361-
throw new MacroAbortError(
362-
`Macro '${macroChoice.name}' targeted script '${selectorMatch.command.name}', but that script does not export '${selectorMatch.memberAccess.join(
363-
"::",
364-
)}'.`,
365-
);
366-
}
367-
368337
return {
369338
candidate: {
370339
command: selectorMatch.command,
371340
index: selectorMatch.index,
372-
exportsRef,
373-
resolvedMember,
374341
},
375342
memberAccess: selectorMatch.memberAccess,
376343
};
377344
}
378345

379-
const candidates: UserScriptCandidate[] = [];
346+
const candidates: Array<
347+
UserScriptCandidate & { resolvedMember: { found: boolean; value?: unknown } }
348+
> = [];
380349

381350
for (const entry of userScriptCommands) {
382351
const exportsRef = await getUserScript(entry.command, this.app);
383-
const cacheKey = entry.command.path ?? entry.command.id;
384-
if (cacheKey && exportsRef !== undefined && exportsRef !== null) {
385-
preloadedScripts.set(cacheKey, exportsRef);
386-
}
387-
388352
candidates.push({
389353
command: entry.command,
390354
index: entry.index,

tests/e2e/macro-member-access.test.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,8 @@ import type {
1515

1616
const VAULT = "dev";
1717
const PLUGIN_ID = "quickadd";
18-
const WAIT_OPTS = { timeoutMs: 10_000, intervalMs: 200 };
18+
const waitTimeoutMs = Number(process.env.E2E_TIMEOUT_MS) || 15_000;
19+
const WAIT_OPTS = { timeoutMs: waitTimeoutMs, intervalMs: 200 };
1920
const TEST_PREFIX = "__qa-test-964-";
2021

2122
let obsidian: ObsidianClient;

0 commit comments

Comments
 (0)