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

This PR fixes the Ability to resolve with nothing issue #389

Merged
merged 13 commits into from
Jun 24, 2024
Merged
62 changes: 62 additions & 0 deletions plugins/async-node/core/src/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,68 @@ const basicFRFWithActions = {
},
};

const asyncNodeTest = async (resolvedValue: any, expectedActionType: string) => {
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', () => {
updateNumber++;
});
});
});

player.start(basicFRFWithActions as any);

let view = (player.getState() as InProgressState).controllers.view.currentView
?.lastUpdate;

expect(view).toBeDefined();
expect(view?.actions[0].asset.type).toBe('action');
expect(view?.actions[1]).toBeUndefined();
expect(updateNumber).toBe(1);

await waitFor(() => {
expect(deferredResolve).toBeDefined();
});

if (deferredResolve) {
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');
sakuntala-motukuri marked this conversation as resolved.
Show resolved Hide resolved
expect(view?.actions[1]?.asset.type).toBe(expectedActionType);
sakuntala-motukuri marked this conversation as resolved.
Show resolved Hide resolved
};

test('should return current node view when the resolved node is null', async () => {
await asyncNodeTest(null, undefined);
});

test('should return current node view when the resolved node is undefined', async () => {
await asyncNodeTest(undefined, undefined);
});

test("replaces async nodes with provided node", async () => {
const plugin = new AsyncNodePlugin({
plugins: [new AsyncNodePluginPlugin()],
Expand Down
9 changes: 7 additions & 2 deletions plugins/async-node/core/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -166,11 +166,16 @@ export class AsyncNodePluginPlugin implements AsyncNodeViewPlugin {
options.parseNode && result
? options.parseNode(result)
: undefined;

if (parsedNode) {
this.resolvedMapping.set(node.id, parsedNode);
view.updateAsync();
} else {
// If parsedNode is undefined, update the resolvedMapping with the existing node (supporting null | undefined) as of now
// This is to ensure that the node is not resolved again
if (!this.resolvedMapping.has(node.id)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This plugin, if registered, will live as long as the player instance it's registered to is still active.
That means as we repeated update UI, this check will only be true once.

I'm not 100% sure your original solution won't work, i'd write some tests to make sure the solution is okay through multiple updates

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed the business logic to such the node is resolved irrespective of the type which ensures that the consumer can expect the node to be resolved irrespective of the input they provide us with. Also updated respective test case such that we ensure nothing is resolved and also that the resolution happens only once and after the promise has been resolved.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should check that returning the same async node doesn't impact future updates, I think you could do so by manually calling the update function in the resolver, see what happens if you try to update again after the resolve with null/undefined has been processed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you're not looking at the updated code .. This is the updated code
image

this.resolvedMapping.set(node.id, node);
}
}
view.updateAsync();
});

return node;
Expand Down