Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ability to remove a resolved async node #404

Merged
merged 19 commits into from
Jul 24, 2024
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
50591e0
Rebased onto main branch for ability to remove a resolved async node
sakuntala-motukuri Jul 16, 2024
12f9d46
Merge branch 'main' into bazel-6-Ability-to-remove-a-resolved-async-node
sakuntala-motukuri Jul 16, 2024
9e27f2d
Updated code with callback mechanism and updated respective test cases
sakuntala-motukuri Jul 18, 2024
ac7fec1
Merge branch 'main' into bazel-6-Ability-to-remove-a-resolved-async-node
sakuntala-motukuri Jul 18, 2024
a198f1f
Fixed PR review comments
sakuntala-motukuri Jul 18, 2024
fc73d00
Fixed PR review comments second time
sakuntala-motukuri Jul 18, 2024
b95b53d
updated test to be in a waitfor
sakuntala-motukuri Jul 18, 2024
49a4d1d
removed AsyncNodePlugin initialisation to satiate the test accordingly
sakuntala-motukuri Jul 18, 2024
db383d8
removed unnecessary code and tests
sakuntala-motukuri Jul 18, 2024
6764f0d
Updated docs for async-node.mdx
sakuntala-motukuri Jul 19, 2024
43e0edb
Updated suggested changes
sakuntala-motukuri Jul 19, 2024
0030479
Fixed additional review comments
sakuntala-motukuri Jul 22, 2024
5c556c9
Merge branch 'main' into bazel-6-Ability-to-remove-a-resolved-async-node
sakuntala-motukuri Jul 22, 2024
3bb57e1
Code cleanup as per ask by Tony
sakuntala-motukuri Jul 22, 2024
c0d9dff
Merge branch 'main' into bazel-6-Ability-to-remove-a-resolved-async-node
sakuntala-motukuri Jul 22, 2024
8403522
Updated naming convention
sakuntala-motukuri Jul 22, 2024
6cd0a7f
view instance was resolving to null due to which the tests were faili…
sakuntala-motukuri Jul 23, 2024
428a217
Merge branch 'main' into bazel-6-Ability-to-remove-a-resolved-async-node
sakuntala-motukuri Jul 23, 2024
44f5bca
bind
brocollie08 Jul 24, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
167 changes: 157 additions & 10 deletions plugins/async-node/core/src/index.test.ts
Original file line number Diff line number Diff line change
@@ -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";
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -84,7 +93,7 @@ const asyncNodeTest = async (resolvedValue: any) => {
}

await waitFor(() => {
expect(updateNumber).toBe(2);
expect(updateNumber).toBe(1);
});

view = (player.getState() as InProgressState).controllers.view.currentView
Expand All @@ -93,10 +102,14 @@ const asyncNodeTest = async (resolvedValue: any) => {
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 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(3);
expect(updateNumber).toBe(1);
});

view = (player.getState() as InProgressState).controllers.view.currentView
Expand All @@ -114,6 +127,140 @@ test("should return current node view when the resolved node is undefined", asyn
await asyncNodeTest(undefined);
});

test("can handle multiple updates through callback mechanism", async () => {
const plugin = new AsyncNodePlugin({
plugins: [new AsyncNodePluginPlugin()],
});

let deferredResolve: ((value: any) => void) | undefined;

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] });

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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this not checking for something out of bounds?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since view is just a record, and the value of actions can be of type any, could you get actions[2] being undefined but actions[3] being well defined? If so, does it make more sense to check size rather than actions[2] specifically

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean I think we shouldn't check for view?.actions[2] at all because there's no significance to it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyhow view?.actions[2] would be undefined , but yes as you said it makes no sense checking for this at this point of time as view?.actions[2] is not expected at all. Removed it


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("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));

sakuntala-motukuri marked this conversation as resolved.
Show resolved Hide resolved
// Verify that the updateNumber is still 0, indicating that the async node was not resolved
expect(updateNumber).toBe(0);
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I'm a bit skeptical with this test -- it doesn't look like you have the setup to update updateNumber on view updates. And even then, I would expect it to update at least once for when the initial view resolution occurs when you start the Player

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes I removed it on purpose , as I wanted to check if the view would still be resolved even if the baseplugin instance doesn't exist , do we still want the view to update ?


test("replaces async nodes with provided node", async () => {
const plugin = new AsyncNodePlugin({
plugins: [new AsyncNodePluginPlugin()],
Expand Down Expand Up @@ -346,7 +493,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) => {
sakuntala-motukuri marked this conversation as resolved.
Show resolved Hide resolved
plugin.hooks.onAsyncNode.tap("test", async (node: Node.Node) => {
return new Promise((resolve) => {
deferredResolve = resolve;
});
Expand Down
63 changes: 44 additions & 19 deletions plugins/async-node/core/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
Parser,
ViewPlugin,
Resolver,
Resolve,
} from "@player-ui/player";
import { AsyncParallelBailHook } from "tapable-ts";
import queueMicrotask from "queue-microtask";
Expand All @@ -24,7 +25,7 @@
/** 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>;
}

/**
Expand All @@ -44,7 +45,10 @@
}

public readonly hooks = {
onAsyncNode: new AsyncParallelBailHook<[Node.Async], any>(),
onAsyncNode: new AsyncParallelBailHook<
[Node.Async, (result: any) => void],
any
>(),
};

name = "AsyncNode";
Expand All @@ -61,7 +65,10 @@
}

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";
Expand All @@ -70,6 +77,21 @@

private currentView: ViewInstance | undefined;

private handleAsyncUpdate(
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;
}
Expand Down Expand Up @@ -127,14 +149,16 @@
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;

if (parsedNode) {
this.resolvedMapping.set(node.id, parsedNode);
this.currentView?.updateAsync();
if (!this.basePlugin) {
return;

Check warning on line 153 in plugins/async-node/core/src/index.ts

View check run for this annotation

Codecov / codecov/patch

plugins/async-node/core/src/index.ts#L152-L153

Added lines #L152 - L153 were not covered by tests
}
const result = await this.basePlugin?.hooks.onAsyncNode.call(
node,
(result) => {
this.handleAsyncUpdate(node, result, options, this.currentView);
},
);
this.handleAsyncUpdate(node, result, options, this.currentView);

Check warning on line 161 in plugins/async-node/core/src/index.ts

View check run for this annotation

Codecov / codecov/patch

plugins/async-node/core/src/index.ts#L155-L161

Added lines #L155 - L161 were not covered by tests
});

return node;
Expand All @@ -161,19 +185,20 @@
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;
}

Check warning on line 190 in plugins/async-node/core/src/index.ts

View check run for this annotation

Codecov / codecov/patch

plugins/async-node/core/src/index.ts#L189-L190

Added lines #L189 - L190 were not covered by tests
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;
});
});
Expand Down