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 6 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
171 changes: 162 additions & 9 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,146 @@ 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()],
});

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 new Promise((resolve) => setTimeout(resolve, 100));

sakuntala-motukuri marked this conversation as resolved.
Show resolved Hide resolved
expect(updateNumber).toBe(1);
expect(deferredResolve).toBeUndefined();
});

test("replaces async nodes with provided node", async () => {
const plugin = new AsyncNodePlugin({
plugins: [new AsyncNodePluginPlugin()],
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