-
Notifications
You must be signed in to change notification settings - Fork 49
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
Changes from 10 commits
50591e0
12f9d46
9e27f2d
ac7fec1
a198f1f
fc73d00
b95b53d
49a4d1d
db383d8
6764f0d
43e0edb
0030479
5c556c9
3bb57e1
c0d9dff
8403522
6cd0a7f
428a217
44f5bca
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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 | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||||||
|
||||||
<PlatformTabs> | ||||||
<core> | ||||||
|
@@ -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({ | |||||
</core> | ||||||
<react> | ||||||
|
||||||
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: | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this should stay There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||||||
|
||||||
```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. | ||||||
|
||||||
</ios> | ||||||
<android> | ||||||
|
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"; | ||
|
@@ -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; | ||
|
||
|
@@ -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 | ||
|
@@ -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 | ||
|
@@ -114,6 +127,90 @@ 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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this not checking for something out of bounds? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. since There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I mean I think we shouldn't check for There was a problem hiding this comment. Choose a reason for hiding this commentThe 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("replaces async nodes with provided node", async () => { | ||
const plugin = new AsyncNodePlugin({ | ||
plugins: [new AsyncNodePluginPlugin()], | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍