From 50591e0b8eceb7b9cf5cdd2d7d3e4e63c57004f4 Mon Sep 17 00:00:00 2001 From: sakuntala_motukuri Date: Tue, 16 Jul 2024 02:53:05 -0400 Subject: [PATCH 01/14] Rebased onto main branch for ability to remove a resolved async node --- plugins/async-node/core/src/index.test.ts | 98 +++++++++++++++-------- plugins/async-node/core/src/index.ts | 31 ++++--- 2 files changed, 86 insertions(+), 43 deletions(-) diff --git a/plugins/async-node/core/src/index.test.ts b/plugins/async-node/core/src/index.test.ts index 74c1e0f4d..81e0df109 100644 --- a/plugins/async-node/core/src/index.test.ts +++ b/plugins/async-node/core/src/index.test.ts @@ -83,20 +83,17 @@ const asyncNodeTest = async (resolvedValue: any) => { deferredResolve(resolvedValue); } - await waitFor(() => { - expect(updateNumber).toBe(2); - }); - view = (player.getState() as InProgressState).controllers.view.currentView ?.lastUpdate; expect(view?.actions[0].asset.type).toBe("action"); expect(view?.actions.length).toBe(1); - viewInstance?.update(); + viewInstance.update(); + //Even after a manual update, the view should not change as we are deleting the resolved node if there is no view update await waitFor(() => { - expect(updateNumber).toBe(3); + expect(updateNumber).toBe(2); }); view = (player.getState() as InProgressState).controllers.view.currentView @@ -114,6 +111,66 @@ test("should return current node view when the resolved node is undefined", asyn await asyncNodeTest(undefined); }); +test("should handle the promise using .then", async () => { + const plugin = new AsyncNodePlugin({ + plugins: [new AsyncNodePluginPlugin()], + }); + + let deferredResolve: ((value: any) => void) | undefined; + + plugin.hooks.onAsyncNode.tap("test", async (node: Node.Node) => { + return new Promise((resolve) => { + deferredResolve = resolve; + }); + }); + let updateNumber = 0; + + const player = new Player({ plugins: [plugin] }); + + player.hooks.viewController.tap("async-node-test", (vc) => { + vc.hooks.view.tap("async-node-test", (view) => { + view.hooks.onUpdate.tap("async-node-test", (update) => { + updateNumber++; + }); + }); + }); + + player.start(basicFRFWithActions as any); + + let view = (player.getState() as InProgressState).controllers.view.currentView + ?.lastUpdate; + + expect(view).toBeDefined(); + expect(view?.actions[1]).toBeUndefined(); + + await waitFor(() => { + expect(updateNumber).toBe(1); + expect(deferredResolve).toBeDefined(); + }); + + if (deferredResolve) { + deferredResolve({ + asset: { + id: "next-label-action", + type: "action", + value: "dummy value", + }, + }); + } + + await waitFor(() => { + expect(updateNumber).toBe(2); + }); + + view = (player.getState() as InProgressState).controllers.view.currentView + ?.lastUpdate; + + expect(view?.actions[0].asset.type).toBe("action"); + expect(view?.actions[1].asset.type).toBe("action"); + expect(view?.actions[2]).toBeUndefined(); + expect(updateNumber).toBe(2); +}); + test("replaces async nodes with provided node", async () => { const plugin = new AsyncNodePlugin({ plugins: [new AsyncNodePluginPlugin()], @@ -346,7 +403,7 @@ test("replaces async nodes with chained multiNodes singular", async () => { let deferredResolve: ((value: any) => void) | undefined; - plugin.hooks.onAsyncNode.tap("test", async (node: Node.Async) => { + plugin.hooks.onAsyncNode.tap("test", async (node: Node.Node) => { return new Promise((resolve) => { deferredResolve = resolve; }); @@ -424,30 +481,3 @@ test("replaces async nodes with chained multiNodes singular", async () => { expect(view?.actions[1].asset.type).toBe("text"); expect(view?.actions[2].asset.type).toBe("text"); }); - -test("should call onAsyncNode hook when async node is encountered", async () => { - const plugin = new AsyncNodePlugin({ - plugins: [new AsyncNodePluginPlugin()], - }); - - let localNode: Node.Async; - plugin.hooks.onAsyncNode.tap("test", async (node: Node.Async) => { - if (node !== null) { - // assigns node value to a local variable - localNode = node; - } - - return new Promise((resolve) => { - resolve("Promise resolved"); - }); - }); - - const player = new Player({ plugins: [plugin] }); - - player.start(basicFRFWithActions as any); - - await waitFor(() => { - expect(localNode.id).toStrictEqual("nodeId"); - expect(localNode.type).toStrictEqual("async"); - }); -}); diff --git a/plugins/async-node/core/src/index.ts b/plugins/async-node/core/src/index.ts index a8f63c051..2dde59896 100644 --- a/plugins/async-node/core/src/index.ts +++ b/plugins/async-node/core/src/index.ts @@ -70,6 +70,8 @@ export class AsyncNodePluginPlugin implements AsyncNodeViewPlugin { private currentView: ViewInstance | undefined; + private pendingUpdates = new Set(); + private isAsync(node: Node.Node | null): node is Node.Async { return node?.type === NodeType.Async; } @@ -151,8 +153,12 @@ export class AsyncNodePluginPlugin implements AsyncNodeViewPlugin { let resolvedNode; if (this.isAsync(node)) { const mappedValue = this.resolvedMapping.get(node.id); + if (this.pendingUpdates.has(node.id)) { + return; // Skip processing if already scheduled + } if (mappedValue) { resolvedNode = mappedValue; + this.pendingUpdates.add(node.id); } } else { resolvedNode = null; @@ -161,19 +167,26 @@ export class AsyncNodePluginPlugin implements AsyncNodeViewPlugin { const newNode = resolvedNode || node; if (!resolvedNode && node?.type === NodeType.Async) { queueMicrotask(async () => { - const result = await this.basePlugin?.hooks.onAsyncNode.call(node); - const parsedNode = - options.parseNode && result - ? options.parseNode(result) - : undefined; - - this.resolvedMapping.set(node.id, parsedNode ? parsedNode : node); - view.updateAsync(); + if (!this.basePlugin) { + return; + } + this.basePlugin?.hooks.onAsyncNode.call(node).then((result) => { + const parsedNode = + options.parseNode && result + ? options.parseNode(result) + : undefined; + + if (parsedNode) { + this.resolvedMapping.set(node.id, parsedNode); + view.updateAsync(); + console.log("pending updates--", this.pendingUpdates.size); + this.pendingUpdates.delete(node.id); + } + }); }); return node; } - return newNode; }); }); From 9e27f2d14869519b3b05c78379227b7b7ac07f7d Mon Sep 17 00:00:00 2001 From: sakuntala_motukuri Date: Wed, 17 Jul 2024 23:30:51 -0400 Subject: [PATCH 02/14] Updated code with callback mechanism and updated respective test cases --- plugins/async-node/core/src/index.test.ts | 68 ++++++++++++++++++----- plugins/async-node/core/src/index.ts | 59 ++++++++++++-------- 2 files changed, 91 insertions(+), 36 deletions(-) diff --git a/plugins/async-node/core/src/index.test.ts b/plugins/async-node/core/src/index.test.ts index 81e0df109..7b1c26a81 100644 --- a/plugins/async-node/core/src/index.test.ts +++ b/plugins/async-node/core/src/index.test.ts @@ -1,4 +1,4 @@ -import { expect, test } from "vitest"; +import { expect, test, vi } from "vitest"; import { Node, InProgressState, ViewInstance } from "@player-ui/player"; import { Player } from "@player-ui/player"; import { waitFor } from "@testing-library/react"; @@ -44,11 +44,20 @@ const asyncNodeTest = async (resolvedValue: any) => { let deferredResolve: ((value: any) => void) | undefined; - plugin.hooks.onAsyncNode.tap("test", async (node: Node.Async) => { - return new Promise((resolve) => { - deferredResolve = resolve; // Promise would be resolved only once - }); - }); + let updateContent: any; + + plugin.hooks.onAsyncNode.tap( + "test", + async (node: Node.Async, update: (content: any) => void) => { + const result = new Promise((resolve) => { + deferredResolve = resolve; // Promise would be resolved only once + }); + + updateContent = update; + // Return the result to follow the same mechanism as before + return result; + }, + ); let updateNumber = 0; @@ -83,17 +92,24 @@ const asyncNodeTest = async (resolvedValue: any) => { deferredResolve(resolvedValue); } + await waitFor(() => { + expect(updateNumber).toBe(1); + }); + view = (player.getState() as InProgressState).controllers.view.currentView ?.lastUpdate; expect(view?.actions[0].asset.type).toBe("action"); expect(view?.actions.length).toBe(1); - viewInstance.update(); + // Consumer responds with null/undefined + if (deferredResolve) { + updateContent(resolvedValue); + } - //Even after a manual update, the view should not change as we are deleting the resolved node if there is no view update + //Even after an update, the view should not change as we are deleting the resolved node if there is no view update await waitFor(() => { - expect(updateNumber).toBe(2); + expect(updateNumber).toBe(1); }); view = (player.getState() as InProgressState).controllers.view.currentView @@ -118,11 +134,21 @@ test("should handle the promise using .then", async () => { let deferredResolve: ((value: any) => void) | undefined; - plugin.hooks.onAsyncNode.tap("test", async (node: Node.Node) => { - return new Promise((resolve) => { - deferredResolve = resolve; - }); - }); + let updateContent: any; + + plugin.hooks.onAsyncNode.tap( + "test", + async (node: Node.Async, update: (content: any) => void) => { + const result = new Promise((resolve) => { + deferredResolve = resolve; // Promise would be resolved only once + }); + + updateContent = update; + // Return the result to follow the same mechanism as before + return result; + }, + ); + let updateNumber = 0; const player = new Player({ plugins: [plugin] }); @@ -169,6 +195,20 @@ test("should handle the promise using .then", async () => { expect(view?.actions[1].asset.type).toBe("action"); expect(view?.actions[2]).toBeUndefined(); expect(updateNumber).toBe(2); + + if (deferredResolve) { + updateContent(null); + } + + await waitFor(() => { + expect(updateNumber).toBe(3); + }); + + view = (player.getState() as InProgressState).controllers.view.currentView + ?.lastUpdate; + + expect(view?.actions[0].asset.type).toBe("action"); + expect(view?.actions[1]).toBeUndefined(); }); test("replaces async nodes with provided node", async () => { diff --git a/plugins/async-node/core/src/index.ts b/plugins/async-node/core/src/index.ts index 2dde59896..ddf690d69 100644 --- a/plugins/async-node/core/src/index.ts +++ b/plugins/async-node/core/src/index.ts @@ -8,6 +8,7 @@ import type { Parser, ViewPlugin, Resolver, + Resolve, } from "@player-ui/player"; import { AsyncParallelBailHook } from "tapable-ts"; import queueMicrotask from "queue-microtask"; @@ -24,7 +25,7 @@ export interface AsyncNodeViewPlugin extends ViewPlugin { /** Use this to tap into the async node plugin hooks */ applyPlugin: (asyncNodePlugin: AsyncNodePlugin) => void; - asyncNode: AsyncParallelBailHook<[Node.Async], any>; + asyncNode: AsyncParallelBailHook<[Node.Async, (result: any) => void], any>; } /** @@ -44,7 +45,10 @@ export class AsyncNodePlugin implements PlayerPlugin { } public readonly hooks = { - onAsyncNode: new AsyncParallelBailHook<[Node.Async], any>(), + onAsyncNode: new AsyncParallelBailHook< + [Node.Async, (result: any) => void], + any + >(), }; name = "AsyncNode"; @@ -61,7 +65,10 @@ export class AsyncNodePlugin implements PlayerPlugin { } export class AsyncNodePluginPlugin implements AsyncNodeViewPlugin { - public asyncNode = new AsyncParallelBailHook<[Node.Async], any>(); + public asyncNode = new AsyncParallelBailHook< + [Node.Async, (result: any) => void], + any + >(); private basePlugin: AsyncNodePlugin | undefined; name = "AsyncNode"; @@ -70,7 +77,20 @@ export class AsyncNodePluginPlugin implements AsyncNodeViewPlugin { private currentView: ViewInstance | undefined; - private pendingUpdates = new Set(); + private updateAsyncFunc( + node: Node.Async, + result: any, + options: Resolve.NodeResolveOptions, + view: ViewInstance | undefined, + ) { + const parsedNode = + options.parseNode && result ? options.parseNode(result) : undefined; + + if (this.resolvedMapping.get(node.id) !== parsedNode) { + this.resolvedMapping.set(node.id, parsedNode ? parsedNode : node); + view?.updateAsync(); + } + } private isAsync(node: Node.Node | null): node is Node.Async { return node?.type === NodeType.Async; @@ -129,7 +149,12 @@ export class AsyncNodePluginPlugin implements AsyncNodeViewPlugin { const newNode = resolvedNode || node; if (!resolvedNode && node?.type === NodeType.Async) { queueMicrotask(async () => { - const result = await this.basePlugin?.hooks.onAsyncNode.call(node); + const result = await this.basePlugin?.hooks.onAsyncNode.call( + node, + (result) => { + this.updateAsyncFunc(node, result, options, this.currentView); + }, + ); const parsedNode = options.parseNode && result ? options.parseNode(result) : undefined; @@ -153,12 +178,8 @@ export class AsyncNodePluginPlugin implements AsyncNodeViewPlugin { let resolvedNode; if (this.isAsync(node)) { const mappedValue = this.resolvedMapping.get(node.id); - if (this.pendingUpdates.has(node.id)) { - return; // Skip processing if already scheduled - } if (mappedValue) { resolvedNode = mappedValue; - this.pendingUpdates.add(node.id); } } else { resolvedNode = null; @@ -170,19 +191,13 @@ export class AsyncNodePluginPlugin implements AsyncNodeViewPlugin { if (!this.basePlugin) { return; } - this.basePlugin?.hooks.onAsyncNode.call(node).then((result) => { - const parsedNode = - options.parseNode && result - ? options.parseNode(result) - : undefined; - - if (parsedNode) { - this.resolvedMapping.set(node.id, parsedNode); - view.updateAsync(); - console.log("pending updates--", this.pendingUpdates.size); - this.pendingUpdates.delete(node.id); - } - }); + const result = await this.basePlugin?.hooks.onAsyncNode.call( + node, + (result) => { + this.updateAsyncFunc(node, result, options, view); + }, + ); + this.updateAsyncFunc(node, result, options, view); }); return node; From a198f1fb2371d5a03c6c4f5a4c4593cec98b4247 Mon Sep 17 00:00:00 2001 From: sakuntala_motukuri Date: Thu, 18 Jul 2024 15:21:00 -0400 Subject: [PATCH 03/14] Fixed PR review comments --- plugins/async-node/core/src/index.test.ts | 79 ++++++++++++++++++++++- plugins/async-node/core/src/index.ts | 19 +++--- 2 files changed, 86 insertions(+), 12 deletions(-) diff --git a/plugins/async-node/core/src/index.test.ts b/plugins/async-node/core/src/index.test.ts index 7b1c26a81..c3d06809b 100644 --- a/plugins/async-node/core/src/index.test.ts +++ b/plugins/async-node/core/src/index.test.ts @@ -127,7 +127,7 @@ test("should return current node view when the resolved node is undefined", asyn await asyncNodeTest(undefined); }); -test("should handle the promise using .then", async () => { +test("can handle multiple updates through callback mechanism", async () => { const plugin = new AsyncNodePlugin({ plugins: [new AsyncNodePluginPlugin()], }); @@ -211,6 +211,56 @@ test("should handle the promise using .then", async () => { expect(view?.actions[1]).toBeUndefined(); }); +test("should not proceed with async node resolution if basePlugin is not set", async () => { + const plugin = new AsyncNodePlugin({ + plugins: [new AsyncNodePluginPlugin()], + }); + //const asyncNodePlugin = new AsyncNodePluginPlugin(); + // Do not set basePlugin to simulate the condition + // asyncNodePluginPlugin.applyPlugin(plugin); // This line is intentionally omitted + + plugin.apply(new Player()); + + const updateNumber = 0; + + const player = new Player({ plugins: [plugin] }); + + // Define a basic flow that includes an async node + const basicFRFWithAsyncNode = { + id: "test-flow", + views: [ + { + id: "my-view", + actions: [ + { + id: "asyncNodeId", + async: "true", + }, + ], + }, + ], + navigation: { + BEGIN: "FLOW_1", + FLOW_1: { + startState: "VIEW_1", + VIEW_1: { + state_type: "VIEW", + ref: "my-view", + transitions: {}, + }, + }, + }, + }; + + player.start(basicFRFWithAsyncNode as any); + + // Wait for any asynchronous operations to potentially complete + await new Promise((resolve) => setTimeout(resolve, 100)); + + // Verify that the updateNumber is still 0, indicating that the async node was not resolved + expect(updateNumber).toBe(0); +}); + test("replaces async nodes with provided node", async () => { const plugin = new AsyncNodePlugin({ plugins: [new AsyncNodePluginPlugin()], @@ -521,3 +571,30 @@ test("replaces async nodes with chained multiNodes singular", async () => { expect(view?.actions[1].asset.type).toBe("text"); expect(view?.actions[2].asset.type).toBe("text"); }); + +test("should call onAsyncNode hook when async node is encountered", async () => { + const plugin = new AsyncNodePlugin({ + plugins: [new AsyncNodePluginPlugin()], + }); + + let localNode: Node.Async; + plugin.hooks.onAsyncNode.tap("test", async (node: Node.Async) => { + if (node !== null) { + // assigns node value to a local variable + localNode = node; + } + + return new Promise((resolve) => { + resolve("Promise resolved"); + }); + }); + + const player = new Player({ plugins: [plugin] }); + + player.start(basicFRFWithActions as any); + + await waitFor(() => { + expect(localNode.id).toStrictEqual("nodeId"); + expect(localNode.type).toStrictEqual("async"); + }); +}); diff --git a/plugins/async-node/core/src/index.ts b/plugins/async-node/core/src/index.ts index ddf690d69..565a8d2ab 100644 --- a/plugins/async-node/core/src/index.ts +++ b/plugins/async-node/core/src/index.ts @@ -77,7 +77,7 @@ export class AsyncNodePluginPlugin implements AsyncNodeViewPlugin { private currentView: ViewInstance | undefined; - private updateAsyncFunc( + private handleAsyncUpdate( node: Node.Async, result: any, options: Resolve.NodeResolveOptions, @@ -149,19 +149,16 @@ export class AsyncNodePluginPlugin implements AsyncNodeViewPlugin { const newNode = resolvedNode || node; if (!resolvedNode && node?.type === NodeType.Async) { queueMicrotask(async () => { + if (!this.basePlugin) { + return; + } const result = await this.basePlugin?.hooks.onAsyncNode.call( node, (result) => { - this.updateAsyncFunc(node, result, options, this.currentView); + this.handleAsyncUpdate(node, result, options, this.currentView); }, ); - const parsedNode = - options.parseNode && result ? options.parseNode(result) : undefined; - - if (parsedNode) { - this.resolvedMapping.set(node.id, parsedNode); - this.currentView?.updateAsync(); - } + this.handleAsyncUpdate(node, result, options, this.currentView); }); return node; @@ -194,10 +191,10 @@ export class AsyncNodePluginPlugin implements AsyncNodeViewPlugin { const result = await this.basePlugin?.hooks.onAsyncNode.call( node, (result) => { - this.updateAsyncFunc(node, result, options, view); + this.handleAsyncUpdate(node, result, options, view); }, ); - this.updateAsyncFunc(node, result, options, view); + this.handleAsyncUpdate(node, result, options, view); }); return node; From fc73d00467962663623a000c4975713a0c5f9fda Mon Sep 17 00:00:00 2001 From: sakuntala_motukuri Date: Thu, 18 Jul 2024 16:02:16 -0400 Subject: [PATCH 04/14] Fixed PR review comments second time --- plugins/async-node/core/src/index.test.ts | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/plugins/async-node/core/src/index.test.ts b/plugins/async-node/core/src/index.test.ts index c3d06809b..046337f8e 100644 --- a/plugins/async-node/core/src/index.test.ts +++ b/plugins/async-node/core/src/index.test.ts @@ -215,16 +215,22 @@ test("should not proceed with async node resolution if basePlugin is not set", a const plugin = new AsyncNodePlugin({ plugins: [new AsyncNodePluginPlugin()], }); - //const asyncNodePlugin = new AsyncNodePluginPlugin(); - // Do not set basePlugin to simulate the condition - // asyncNodePluginPlugin.applyPlugin(plugin); // This line is intentionally omitted plugin.apply(new Player()); - const updateNumber = 0; + let updateNumber = 0; + let deferredResolve: ((value: any) => void) | undefined; const player = new Player({ plugins: [plugin] }); + player.hooks.viewController.tap("async-node-test", (vc) => { + vc.hooks.view.tap("async-node-test", (view) => { + view.hooks.onUpdate.tap("async-node-test", (update) => { + updateNumber++; + }); + }); + }); + // Define a basic flow that includes an async node const basicFRFWithAsyncNode = { id: "test-flow", @@ -257,8 +263,8 @@ test("should not proceed with async node resolution if basePlugin is not set", a // Wait for any asynchronous operations to potentially complete await new Promise((resolve) => setTimeout(resolve, 100)); - // Verify that the updateNumber is still 0, indicating that the async node was not resolved - expect(updateNumber).toBe(0); + expect(updateNumber).toBe(1); + expect(deferredResolve).toBeUndefined(); }); test("replaces async nodes with provided node", async () => { @@ -493,7 +499,7 @@ test("replaces async nodes with chained multiNodes singular", async () => { let deferredResolve: ((value: any) => void) | undefined; - plugin.hooks.onAsyncNode.tap("test", async (node: Node.Node) => { + plugin.hooks.onAsyncNode.tap("test", async (node: Node.Async) => { return new Promise((resolve) => { deferredResolve = resolve; }); From b95b53dcae519af8fc7dbc92f15aeba3b21faf7c Mon Sep 17 00:00:00 2001 From: sakuntala_motukuri Date: Thu, 18 Jul 2024 16:37:10 -0400 Subject: [PATCH 05/14] updated test to be in a waitfor --- plugins/async-node/core/src/index.test.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/plugins/async-node/core/src/index.test.ts b/plugins/async-node/core/src/index.test.ts index 046337f8e..e3d690aa7 100644 --- a/plugins/async-node/core/src/index.test.ts +++ b/plugins/async-node/core/src/index.test.ts @@ -261,10 +261,10 @@ test("should not proceed with async node resolution if basePlugin is not set", a player.start(basicFRFWithAsyncNode as any); // Wait for any asynchronous operations to potentially complete - await new Promise((resolve) => setTimeout(resolve, 100)); - - expect(updateNumber).toBe(1); - expect(deferredResolve).toBeUndefined(); + await waitFor(() => { + expect(updateNumber).toBe(1); + expect(deferredResolve).toBeUndefined(); + }); }); test("replaces async nodes with provided node", async () => { From 49a4d1d497ed60f8e3c5e24cbb59342a486434da Mon Sep 17 00:00:00 2001 From: sakuntala_motukuri Date: Thu, 18 Jul 2024 18:23:56 -0400 Subject: [PATCH 06/14] removed AsyncNodePlugin initialisation to satiate the test accordingly --- plugins/async-node/core/src/index.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/plugins/async-node/core/src/index.test.ts b/plugins/async-node/core/src/index.test.ts index e3d690aa7..d769f6fd9 100644 --- a/plugins/async-node/core/src/index.test.ts +++ b/plugins/async-node/core/src/index.test.ts @@ -211,9 +211,9 @@ test("can handle multiple updates through callback mechanism", async () => { expect(view?.actions[1]).toBeUndefined(); }); -test("should not proceed with async node resolution if basePlugin is not set", async () => { +test.only("should not proceed with async node resolution if basePlugin is not set", async () => { const plugin = new AsyncNodePlugin({ - plugins: [new AsyncNodePluginPlugin()], + plugins: [], }); plugin.apply(new Player()); From db383d8b8bf5f233a0a363170bded9d4e557aed3 Mon Sep 17 00:00:00 2001 From: sakuntala_motukuri Date: Thu, 18 Jul 2024 19:06:33 -0400 Subject: [PATCH 07/14] removed unnecessary code and tests --- plugins/async-node/core/src/index.test.ts | 56 ----------------------- plugins/async-node/core/src/index.ts | 6 --- 2 files changed, 62 deletions(-) diff --git a/plugins/async-node/core/src/index.test.ts b/plugins/async-node/core/src/index.test.ts index d769f6fd9..821cae115 100644 --- a/plugins/async-node/core/src/index.test.ts +++ b/plugins/async-node/core/src/index.test.ts @@ -211,62 +211,6 @@ test("can handle multiple updates through callback mechanism", async () => { expect(view?.actions[1]).toBeUndefined(); }); -test.only("should not proceed with async node resolution if basePlugin is not set", async () => { - const plugin = new AsyncNodePlugin({ - plugins: [], - }); - - plugin.apply(new Player()); - - let updateNumber = 0; - let deferredResolve: ((value: any) => void) | undefined; - - const player = new Player({ plugins: [plugin] }); - - player.hooks.viewController.tap("async-node-test", (vc) => { - vc.hooks.view.tap("async-node-test", (view) => { - view.hooks.onUpdate.tap("async-node-test", (update) => { - updateNumber++; - }); - }); - }); - - // Define a basic flow that includes an async node - const basicFRFWithAsyncNode = { - id: "test-flow", - views: [ - { - id: "my-view", - actions: [ - { - id: "asyncNodeId", - async: "true", - }, - ], - }, - ], - navigation: { - BEGIN: "FLOW_1", - FLOW_1: { - startState: "VIEW_1", - VIEW_1: { - state_type: "VIEW", - ref: "my-view", - transitions: {}, - }, - }, - }, - }; - - player.start(basicFRFWithAsyncNode as any); - - // Wait for any asynchronous operations to potentially complete - await waitFor(() => { - expect(updateNumber).toBe(1); - expect(deferredResolve).toBeUndefined(); - }); -}); - test("replaces async nodes with provided node", async () => { const plugin = new AsyncNodePlugin({ plugins: [new AsyncNodePluginPlugin()], diff --git a/plugins/async-node/core/src/index.ts b/plugins/async-node/core/src/index.ts index 565a8d2ab..5ba4faf16 100644 --- a/plugins/async-node/core/src/index.ts +++ b/plugins/async-node/core/src/index.ts @@ -149,9 +149,6 @@ export class AsyncNodePluginPlugin implements AsyncNodeViewPlugin { const newNode = resolvedNode || node; if (!resolvedNode && node?.type === NodeType.Async) { queueMicrotask(async () => { - if (!this.basePlugin) { - return; - } const result = await this.basePlugin?.hooks.onAsyncNode.call( node, (result) => { @@ -185,9 +182,6 @@ export class AsyncNodePluginPlugin implements AsyncNodeViewPlugin { const newNode = resolvedNode || node; if (!resolvedNode && node?.type === NodeType.Async) { queueMicrotask(async () => { - if (!this.basePlugin) { - return; - } const result = await this.basePlugin?.hooks.onAsyncNode.call( node, (result) => { From 6764f0dd329cf65f70e34bbc47237281b12fc490 Mon Sep 17 00:00:00 2001 From: sakuntala_motukuri Date: Fri, 19 Jul 2024 16:15:53 -0400 Subject: [PATCH 08/14] Updated docs for async-node.mdx --- docs/site/pages/plugins/async-node.mdx | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/docs/site/pages/plugins/async-node.mdx b/docs/site/pages/plugins/async-node.mdx index 86cf19de6..d62c99348 100644 --- a/docs/site/pages/plugins/async-node.mdx +++ b/docs/site/pages/plugins/async-node.mdx @@ -32,8 +32,9 @@ In the example below, node with the id "some-async-node" will not be rendered on The `AsyncNodePlugin` exposes an `onAsyncNode` hook on all platforms. The `onAsyncNode` hook will be invoked with the current node when the plugin is available and an `AsyncNode` is detected during the resolve process. The node used to call the hook with could contain metadata according to content spec. -User should tap into the `onAsyncNode` hook to examine the node's metadata before making a decision on what to replace the async node with. The return could be a single asset node or an array of asset nodes. +User should tap into the `onAsyncNode` hook to examine the node's metadata before making a decision on what to replace the async node with. The return could be a single asset node or an array of asset nodes, or the return could be even a null|undefined if the async node is no longer relevant. +Returning a value in the above context enables uses cases where the async node only needs to be resolved once. For use cases where the async node needs to be updated multiple times, the onAsyncNode hook provides a second callback argument that can be used to update the value multiple times. For example, if the async node is used to represent some placeholder for toasts, or notifications, the async node handler could initially resolve with some content, and then update with null after some time to remove those views. ### Continuous Streaming @@ -42,7 +43,8 @@ This means there must be a constant renewal of new `AsyncNode`s after the previo ## Usage -The AsyncNodePlugin itself accepts a list of plugins and should take the AsyncNodePluginPlugin to be passed in. The AsyncNodePluginPlugin also comes from the `'@player-ui/async-node-plugin'` and contains the resolver and parser functionality +The `AsyncNodePlugin` itself accepts an options object with a `plugins` array, enabling the integration of multiple view plugins for extended functionality. +The `AsyncNodePluginPlugin` is provided as a default way of handling asset-async nodes, it is just one handler for one possible way of using async nodes. If the default behaviour does not align with the desired usage, users are able to provide their own implementation of the handler in the form of a plugin to be passed to the base `AsyncNodePlugin`.The `AsyncNodePluginPlugin` also comes from the `'@player-ui/async-node-plugin'` and contains the resolver and parser functionality @@ -63,6 +65,18 @@ asyncNodePlugin.hooks.onAsyncNode.tap('handleAsync', async (node: Node.Node) => // Determine what to return to be parsed into a concrete UI asset }); +// For use cases where the async node needs to be updated multiple times + +asyncNodePlugin.hooks.onAsyncNode.tap("toast-provider", async (node: Node.Async, update: (content) => void) => { + ... + // do some async task to get content + const toastContent = await makeToastFor(node.id); + // set timer for 5 seconds to remove the toast content from the view + setTimeout(() => update(null), 5000); + // uses same mechanism as before + return toastContent; +}); + const player = new Player({ plugins: [ asyncNodePlugin @@ -72,7 +86,7 @@ const player = new Player({ -The `react` version of the AsyncNodePlugin is identical to using the core plugin. Refer to core usage for handler configuration: +The `react` version of the AsyncNodePluginPlugin is identical to using the core plugin. Refer to core usage for handler configuration: ```js import { ReactPlayer } from '@player-ui/react'; @@ -164,7 +178,7 @@ return .multiNode([ Note: the AsyncNode struct is already defined in the plugin with the `async` property defaulted to true so only `id` needs to be passed in -As a convenience to the user, the AsyncNodePlugin just takes a callback which has the content to be returned, this is provided to the plugin which calls the the `onAsyncNode` hook tap method +As a convenience to the user, the AsyncNodePlugin just takes a callback which has the content to be returned, this is provided to the plugin which calls the the `onAsyncNode` hook tap method. The return could be a single asset node or an array of asset nodes, or null if the async node is no longer relevant. From 43e0edb9fba38ae822761c17cc8cbb10387cd44d Mon Sep 17 00:00:00 2001 From: sakuntala_motukuri Date: Fri, 19 Jul 2024 16:54:00 -0400 Subject: [PATCH 09/14] Updated suggested changes --- docs/site/pages/plugins/async-node.mdx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/site/pages/plugins/async-node.mdx b/docs/site/pages/plugins/async-node.mdx index d62c99348..f7a3f3b28 100644 --- a/docs/site/pages/plugins/async-node.mdx +++ b/docs/site/pages/plugins/async-node.mdx @@ -44,7 +44,7 @@ This means there must be a constant renewal of new `AsyncNode`s after the previo ## Usage The `AsyncNodePlugin` itself accepts an options object with a `plugins` array, enabling the integration of multiple view plugins for extended functionality. -The `AsyncNodePluginPlugin` is provided as a default way of handling asset-async nodes, it is just one handler for one possible way of using async nodes. If the default behaviour does not align with the desired usage, users are able to provide their own implementation of the handler in the form of a plugin to be passed to the base `AsyncNodePlugin`.The `AsyncNodePluginPlugin` also comes from the `'@player-ui/async-node-plugin'` and contains the resolver and parser functionality +The `AsyncNodePluginPlugin` is provided as a default way of handling asset-async nodes, it is just one handler for one possible way of using async nodes. If the default behavior does not align with the desired usage, users are able to provide their own implementation of the handler in the form of a plugin to be passed to the base `AsyncNodePlugin`. The `AsyncNodePluginPlugin` also comes from the `'@player-ui/async-node-plugin'` and contains the resolver and parser functionality. @@ -86,7 +86,7 @@ const player = new Player({ -The `react` version of the AsyncNodePluginPlugin is identical to using the core plugin. Refer to core usage for handler configuration: +The `react` version of the AsyncNodePlugin is identical to using the core plugin. Refer to core usage for handler configuration: ```js import { ReactPlayer } from '@player-ui/react'; From 0030479f3ac8959cc6c785619df2d66014250f18 Mon Sep 17 00:00:00 2001 From: sakuntala_motukuri Date: Mon, 22 Jul 2024 14:11:20 -0400 Subject: [PATCH 10/14] Fixed additional review comments --- plugins/async-node/core/src/index.test.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/plugins/async-node/core/src/index.test.ts b/plugins/async-node/core/src/index.test.ts index 821cae115..b2de78a31 100644 --- a/plugins/async-node/core/src/index.test.ts +++ b/plugins/async-node/core/src/index.test.ts @@ -193,7 +193,6 @@ test("can handle multiple updates through callback mechanism", async () => { expect(view?.actions[0].asset.type).toBe("action"); expect(view?.actions[1].asset.type).toBe("action"); - expect(view?.actions[2]).toBeUndefined(); expect(updateNumber).toBe(2); if (deferredResolve) { From 3bb57e1022579074349026a6507baf5df6958d37 Mon Sep 17 00:00:00 2001 From: sakuntala_motukuri Date: Mon, 22 Jul 2024 17:27:57 -0400 Subject: [PATCH 11/14] Code cleanup as per ask by Tony --- plugins/async-node/core/src/index.ts | 103 +++++++++++++-------------- 1 file changed, 48 insertions(+), 55 deletions(-) diff --git a/plugins/async-node/core/src/index.ts b/plugins/async-node/core/src/index.ts index 5ba4faf16..bf39f7dc7 100644 --- a/plugins/async-node/core/src/index.ts +++ b/plugins/async-node/core/src/index.ts @@ -77,6 +77,16 @@ export class AsyncNodePluginPlugin implements AsyncNodeViewPlugin { private currentView: ViewInstance | undefined; + /** + * Updates the node asynchronously based on the result provided. + * This method is responsible for handling the update logic of asynchronous nodes. + * It checks if the node needs to be updated based on the new result and updates the mapping accordingly. + * If an update is necessary, it triggers an asynchronous update on the view. + * @param node The asynchronous node that might be updated. + * @param result The result obtained from resolving the async node. This could be any data structure or value. + * @param options Options provided for node resolution, including a potential parseNode function to process the result. + * @param view The view instance where the node resides. This can be undefined if the view is not currently active. + */ private handleAsyncUpdate( node: Node.Async, result: any, @@ -92,6 +102,42 @@ export class AsyncNodePluginPlugin implements AsyncNodeViewPlugin { } } + /** + * Handles the asynchronous API integration for resolving nodes. + * This method sets up a hook on the resolver's `beforeResolve` event to process async nodes. + * @param resolver The resolver instance to attach the hook to. + * @param view The current view instance. + */ + private handleAsyncApi(resolver: Resolver, view: ViewInstance | undefined) { + resolver.hooks.beforeResolve.tap(this.name, (node, options) => { + let resolvedNode; + if (this.isAsync(node)) { + const mappedValue = this.resolvedMapping.get(node.id); + if (mappedValue) { + resolvedNode = mappedValue; + } + } else { + resolvedNode = null; + } + + const newNode = resolvedNode || node; + if (!resolvedNode && node?.type === NodeType.Async) { + queueMicrotask(async () => { + const result = await this.basePlugin?.hooks.onAsyncNode.call( + node, + (result) => { + this.handleAsyncUpdate(node, result, options, view); + }, + ); + this.handleAsyncUpdate(node, result, options, view); + }); + + return node; + } + return newNode; + }); + } + private isAsync(node: Node.Node | null): node is Node.Async { return node?.type === NodeType.Async; } @@ -135,66 +181,13 @@ export class AsyncNodePluginPlugin implements AsyncNodeViewPlugin { } applyResolverHooks(resolver: Resolver) { - resolver.hooks.beforeResolve.tap(this.name, (node, options) => { - let resolvedNode; - if (this.isAsync(node)) { - const mappedValue = this.resolvedMapping.get(node.id); - if (mappedValue) { - resolvedNode = mappedValue; - } - } else { - resolvedNode = null; - } - - const newNode = resolvedNode || node; - if (!resolvedNode && node?.type === NodeType.Async) { - queueMicrotask(async () => { - const result = await this.basePlugin?.hooks.onAsyncNode.call( - node, - (result) => { - this.handleAsyncUpdate(node, result, options, this.currentView); - }, - ); - this.handleAsyncUpdate(node, result, options, this.currentView); - }); - - return node; - } - - return newNode; - }); + this.handleAsyncApi(resolver, this.currentView); } apply(view: ViewInstance): void { view.hooks.parser.tap("template", this.applyParser.bind(this)); view.hooks.resolver.tap("template", (resolver) => { - resolver.hooks.beforeResolve.tap(this.name, (node, options) => { - let resolvedNode; - if (this.isAsync(node)) { - const mappedValue = this.resolvedMapping.get(node.id); - if (mappedValue) { - resolvedNode = mappedValue; - } - } else { - resolvedNode = null; - } - - const newNode = resolvedNode || node; - if (!resolvedNode && node?.type === NodeType.Async) { - queueMicrotask(async () => { - const result = await this.basePlugin?.hooks.onAsyncNode.call( - node, - (result) => { - this.handleAsyncUpdate(node, result, options, view); - }, - ); - this.handleAsyncUpdate(node, result, options, view); - }); - - return node; - } - return newNode; - }); + this.handleAsyncApi(resolver, view); }); } From 8403522f019d0029a2b714e44ecaa580aac1bce5 Mon Sep 17 00:00:00 2001 From: sakuntala_motukuri Date: Mon, 22 Jul 2024 18:11:13 -0400 Subject: [PATCH 12/14] Updated naming convention --- plugins/async-node/core/src/index.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/plugins/async-node/core/src/index.ts b/plugins/async-node/core/src/index.ts index bf39f7dc7..9b9abe2bd 100644 --- a/plugins/async-node/core/src/index.ts +++ b/plugins/async-node/core/src/index.ts @@ -108,7 +108,7 @@ export class AsyncNodePluginPlugin implements AsyncNodeViewPlugin { * @param resolver The resolver instance to attach the hook to. * @param view The current view instance. */ - private handleAsyncApi(resolver: Resolver, view: ViewInstance | undefined) { + private applyResolver(resolver: Resolver, view: ViewInstance | undefined) { resolver.hooks.beforeResolve.tap(this.name, (node, options) => { let resolvedNode; if (this.isAsync(node)) { @@ -181,13 +181,13 @@ export class AsyncNodePluginPlugin implements AsyncNodeViewPlugin { } applyResolverHooks(resolver: Resolver) { - this.handleAsyncApi(resolver, this.currentView); + this.applyResolver(resolver, this.currentView); } apply(view: ViewInstance): void { view.hooks.parser.tap("template", this.applyParser.bind(this)); view.hooks.resolver.tap("template", (resolver) => { - this.handleAsyncApi(resolver, view); + this.applyResolver(resolver, view); }); } From 6cd0a7fd9f270049f8db4476a6fad12651258053 Mon Sep 17 00:00:00 2001 From: sakuntala_motukuri Date: Tue, 23 Jul 2024 13:26:24 -0400 Subject: [PATCH 13/14] view instance was resolving to null due to which the tests were failing , fixed this issue now --- plugins/async-node/core/src/index.ts | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/plugins/async-node/core/src/index.ts b/plugins/async-node/core/src/index.ts index 9b9abe2bd..fdf76e69f 100644 --- a/plugins/async-node/core/src/index.ts +++ b/plugins/async-node/core/src/index.ts @@ -106,9 +106,9 @@ export class AsyncNodePluginPlugin implements AsyncNodeViewPlugin { * Handles the asynchronous API integration for resolving nodes. * This method sets up a hook on the resolver's `beforeResolve` event to process async nodes. * @param resolver The resolver instance to attach the hook to. - * @param view The current view instance. + * @param view */ - private applyResolver(resolver: Resolver, view: ViewInstance | undefined) { + applyResolver(resolver: Resolver, view: ViewInstance) { resolver.hooks.beforeResolve.tap(this.name, (node, options) => { let resolvedNode; if (this.isAsync(node)) { @@ -180,15 +180,11 @@ export class AsyncNodePluginPlugin implements AsyncNodeViewPlugin { ); } - applyResolverHooks(resolver: Resolver) { - this.applyResolver(resolver, this.currentView); - } - apply(view: ViewInstance): void { view.hooks.parser.tap("template", this.applyParser.bind(this)); - view.hooks.resolver.tap("template", (resolver) => { - this.applyResolver(resolver, view); - }); + view.hooks.resolver.tap("template", (resolver) => + this.applyResolver(resolver, view), + ); } applyPlugin(asyncNodePlugin: AsyncNodePlugin): void { From 44f5bca959d0c5fd97e0dcf00413ee5546858612 Mon Sep 17 00:00:00 2001 From: brocollie08 Date: Tue, 23 Jul 2024 23:14:20 -0400 Subject: [PATCH 14/14] bind --- plugins/async-node/core/src/index.ts | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/plugins/async-node/core/src/index.ts b/plugins/async-node/core/src/index.ts index fdf76e69f..c87fb2817 100644 --- a/plugins/async-node/core/src/index.ts +++ b/plugins/async-node/core/src/index.ts @@ -108,7 +108,7 @@ export class AsyncNodePluginPlugin implements AsyncNodeViewPlugin { * @param resolver The resolver instance to attach the hook to. * @param view */ - applyResolver(resolver: Resolver, view: ViewInstance) { + applyResolver(resolver: Resolver) { resolver.hooks.beforeResolve.tap(this.name, (node, options) => { let resolvedNode; if (this.isAsync(node)) { @@ -126,10 +126,10 @@ export class AsyncNodePluginPlugin implements AsyncNodeViewPlugin { const result = await this.basePlugin?.hooks.onAsyncNode.call( node, (result) => { - this.handleAsyncUpdate(node, result, options, view); + this.handleAsyncUpdate(node, result, options, this.currentView); }, ); - this.handleAsyncUpdate(node, result, options, view); + this.handleAsyncUpdate(node, result, options, this.currentView); }); return node; @@ -181,10 +181,9 @@ export class AsyncNodePluginPlugin implements AsyncNodeViewPlugin { } apply(view: ViewInstance): void { - view.hooks.parser.tap("template", this.applyParser.bind(this)); - view.hooks.resolver.tap("template", (resolver) => - this.applyResolver(resolver, view), - ); + this.currentView = view; + view.hooks.parser.tap("async", this.applyParser.bind(this)); + view.hooks.resolver.tap("async", this.applyResolver.bind(this)); } applyPlugin(asyncNodePlugin: AsyncNodePlugin): void {