update core async node plugins to catch and handle errors by tmarmer · Pull Request #666 · player-ui/player

tmarmer · 2025-06-17T17:56:45Z

Update the AsyncNodePlugin to include a new hook onAsyncNodeError
- The intent of this hook is to catch all errors in onAsyncNode hook and allow for a fallback node to be inserted in case of an error.
Update the AsyncNodePluginPlugin to call the onAsyncNodeError hook when it fails to handle the async node.
If the call to onAsyncNodeError does not return a new node or null, then the error is used to fail the current player flow.
- This is different from the existing behaviour, where the error is left uncaught and just ends up in the browser's console.
- Even if the error is handled, the error is logged to ensure it doesn't get lost.
Fix cyclic deps between //plugins/async-node/core and //plugins/reference-assets/core
Scope the content cache on the async-node to the view that generated it to prevent re-use of the cache across views, flows or player instances.

Change Type (required)

Indicate the type of change your pull request is:

patch
minor
major
N/A

Does your PR have any documentation updates?

Updated docs
No Update needed
Unable to update docs

Release Notes

Update the AsyncNodePlugin to include a new hook onAsyncNodeError for catching errors triggered when resolving async nodes.
Not using the hook or failing to return fallback content will allow the error to bubble up and fail the player state.
Fix cyclic deps between //plugins/async-node/core and //plugins/reference-assets/core
Scope the content cache on the async-node to the view that generated it to prevent re-use of the cache across views, flows or player instances.

📦 Published PR as canary version: 0.11.3--canary.666.24188

Try this version out locally by upgrading relevant packages to 0.11.3--canary.666.24188

tmarmer · 2025-06-17T18:44:46Z

/canary

codecov · 2025-06-17T19:00:39Z

Codecov Report

❌ Patch coverage is 96.36364% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.12%. Comparing base (0f6f3d9) to head (4757762).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
ios/core/Sources/Types/Hooks/Hook.swift	87.50%	3 Missing ⚠️
...ins/async-node/ios/Tests/AsynNodePluginTests.swift	97.18%	2 Missing ⚠️
plugins/async-node/core/src/index.ts	98.55%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #666      +/-   ##
==========================================
+ Coverage   90.05%   90.12%   +0.07%     
==========================================
  Files         333      333              
  Lines       21018    21122     +104     
  Branches     2104     2107       +3     
==========================================
+ Hits        18927    19037     +110     
+ Misses       2073     2067       -6     
  Partials       18       18

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

tmarmer · 2025-06-23T14:24:32Z

    public let onAsyncNode: AsyncHook2<JSValue, JSValue>
+    public let onAsyncNodeError: Hook2<JSValue, JSValue>


Not a fan of this pattern, but I'm not sure the best forward here. Android handles these well and treats them properly as bail hooks with a BailResult and actual types to represent the input and output.

Do we have any documentation on bail hooks? I don't know what that is 😅 .

It's pretty non-trivial to interpret JSValues into actual values, which is I assume why we do it this way.

BUT I'm new and would love to know your thoughts on an alternative.

Do we have any documentation on bail hooks? I don't know what that is 😅 .

Not sure the extent to which this exists for mobile, but for web/core, the hooks use the tapable-ts library based on webpack/tapable, which documents the hook types nicely here.

It's pretty non-trivial to interpret JSValues into actual values, which is I assume why we do it this way.
BUT I'm new and would love to know your thoughts on an alternative.

I guess using a JSValue isn't a huge issue, but it would be nice if we could use anything that can easily be encoded into and decoded from a JSValue and have the base hook type handle that conversion so that the API here is a little more obvious to the consumers what they are getting rather than a JSValue where they need to look at our docs to understand what the input here is.

These hooks also don't provide a type for the return type, so it isn't immediately obvious that you can return a value from these or what that return value should even look like.

The BailResult I mentioned would be a part of that return type for BailHooks since those help indicate whether or not the result should cause the hook to bail or continue. This part might not be too big of a deal since the way the hooks work under the hood is that returning undefined will cause it to continue. Having a BailResult pattern may be helpful even in core though since we may want to enable cases where someone can both bail and return a result of undefined, but I think that warrants a larger discussion.

tmarmer · 2025-06-24T22:31:34Z

/canary

tmarmer · 2025-07-08T15:11:53Z

+                    do {
+                        let result = try await hook(hookValue)
+                        DispatchQueue.main.async {
+                            resolve(result as Any)
+                        }
+                    } catch let e {
+                        let message = e.playerDescription
+                        reject("Async hook threw with error '\(message)'")


I tried to put this do { ... } catch { ... } in the JSUtilities.createPromise to reject on any error, but had issues where the error wasn't bubbling up, I imagine it has something to do with the Task wrapper here. I'm not sure if the createPromise util can account for that while still handling errors elsewhere so I left the error handling here.

If there is a better way to do this, let me know

tmarmer · 2025-07-08T19:02:27Z

/canary

tmarmer · 2025-07-09T20:00:14Z

+const transform: BeforeTransformFunction = (asset) => {
+  const newAsset = asset.children?.[0]?.value;
+
+  if (!newAsset) {
+    return asyncTransform(asset.value.id, "collection");
+  }
+  return asyncTransform(asset.value.id, "collection", newAsset);
+};
+
+const transformPlugin = new AssetTransformCorePlugin(
+  new Registry([[{ type: "chat-message" }, { beforeResolve: transform }]]),
+);
+
+class TestAsyncPlugin implements PlayerPlugin {
+  name = "test-async";
+  apply(player: Player) {
+    player.hooks.view.tap("test-async", (view) => {
+      transformPlugin.apply(view);
+    });
+  }
+}


This might not be the best setup for these tests, but I needed to add these to keep the tests identical to how they functioned with the ReferenceAssetsPlugin.

I think this is probably fine. Creating testing utilities for stuff like this is something we should make a ticket for on the backlog

tmarmer · 2025-07-09T20:46:57Z

/canary

KetanReddy · 2025-07-21T22:28:48Z

IMO this should be a minor release just because its adds a new API but open to make it a patch if there are strong opinions.

… android build

… views

… over js

KetanReddy · 2025-07-21T22:26:12Z

+import kotlinx.serialization.KSerializer
+import kotlinx.serialization.Serializable
+
+@Serializable(with = NodeSyncBailHook2.Serializer::class)


Out of curiosity, any reason we need to call this NodeSyncBailHook2?

I was following the existing naming convention for NodeAsyncParallelBailHook2. I'm not familiar enough with Kotlin to know if there is a better way to share the class name as the number of type arguments grows :X

Ah, I assumed it was just a re-declaration of what the underlying hook was.

the number is just based on the number of args

KetanReddy · 2025-07-25T00:40:28Z

+const transform: BeforeTransformFunction = (asset) => {
+  const newAsset = asset.children?.[0]?.value;
+
+  if (!newAsset) {
+    return asyncTransform(asset.value.id, "collection");
+  }
+  return asyncTransform(asset.value.id, "collection", newAsset);
+};
+
+const transformPlugin = new AssetTransformCorePlugin(
+  new Registry([[{ type: "chat-message" }, { beforeResolve: transform }]]),
+);
+
+class TestAsyncPlugin implements PlayerPlugin {
+  name = "test-async";
+  apply(player: Player) {
+    player.hooks.view.tap("test-async", (view) => {
+      transformPlugin.apply(view);
+    });
+  }
+}


I think this is probably fine. Creating testing utilities for stuff like this is something we should make a ticket for on the backlog

KVSRoyal

The code lgtm from an iOS perspective, in my limited knowledge 😅 . I had a few curiosity questions, but none about things to change.

KVSRoyal · 2025-07-25T19:23:46Z

    test_deps = [
-        ":node_modules/@player-ui/reference-assets-plugin",
        ":node_modules/@player-ui/check-path-plugin",
+        ":node_modules/@player-ui/partial-match-registry",


Curiosity question: What does the partial-match-registry do? Do we have docs on it?

The partial-match-registry exports the classes that handle asset/transform matching. It lets you register an object to match on, typically something like { type: 'asset-type' } but can be as complex as you want and it will handle the logic around matching the asset props to the correct transform or asset.

It was brought in here in this case since I needed a way to remove the reference-assets-plugin as a test dependency in this package, but needed to use the same transform setup that it was getting from that plugin. You can see the use in index.test.ts

KVSRoyal · 2025-07-25T19:32:41Z

    public let onAsyncNode: AsyncHook2<JSValue, JSValue>
+    public let onAsyncNodeError: Hook2<JSValue, JSValue>


Do we have any documentation on bail hooks? I don't know what that is 😅 .

It's pretty non-trivial to interpret JSValues into actual values, which is I assume why we do it this way.

BUT I'm new and would love to know your thoughts on an alternative.

KVSRoyal · 2025-07-25T19:35:56Z

+
+        var count = 0
+
+        let asyncNodePluginPlugin = AsyncNodePluginPlugin()


AsyncNodePluginPlugin is the worst name ever lol. Nothing you can do about it, but I think we should consider renaming that down the line. 😅

We also have something along the line of BeaconPluginPlugin for plugins for the Beacon Plugin.

KVSRoyal · 2025-07-25T19:37:34Z


        if let pluginRef = pluginRef {
-            self.hooks = AsyncNodeHook(onAsyncNode: AsyncHook2(baseValue: pluginRef, name: "onAsyncNode"))
+            self.hooks = AsyncNodeHook(onAsyncNode: AsyncHook2(baseValue: pluginRef, name: "onAsyncNode"), onAsyncNodeError: Hook2(baseValue: pluginRef, name: "onAsyncNodeError"))


Do we have doc on how to use these hooks? 🤔 I don't see any on the docs site.

There are some docs on the async stuff here and I extended it in this PR with an Error Handling section to cover the new hook. Take a look and let me know if there is anything helpful I can add

tmarmer changed the title ~~Async node errors~~ update core async node plugins to catch and handle errors Jun 17, 2025

tmarmer force-pushed the async-node-errors branch from 5672460 to 5d62af8 Compare June 20, 2025 23:42

tmarmer commented Jun 23, 2025

View reviewed changes

tmarmer force-pushed the async-node-errors branch from 2011e24 to bb2c9df Compare July 8, 2025 15:09

tmarmer marked this pull request as ready for review July 8, 2025 15:09

tmarmer requested review from KetanReddy, brocollie08, nancywu1, spentacular and sugarmanz as code owners July 8, 2025 15:09

tmarmer commented Jul 8, 2025

View reviewed changes

tmarmer commented Jul 9, 2025

View reviewed changes

tmarmer force-pushed the async-node-errors branch from 5f0a4e6 to 16c00e5 Compare July 9, 2025 20:07

KetanReddy reviewed Jul 9, 2025

View reviewed changes

Comment thread .bazelrc Outdated

KetanReddy reviewed Jul 9, 2025

View reviewed changes

Comment thread docs/site/src/content/docs/plugins/multiplatform/async-node.mdx Outdated

tmarmer force-pushed the async-node-errors branch from 16bcebb to 2777574 Compare July 18, 2025 18:51

tmarmer requested review from a team as code owners July 18, 2025 18:51

KetanReddy added the minor Increment the minor version when merged label Jul 21, 2025

tmarmer added 4 commits July 22, 2025 14:26

update core async node plugins to catch and handle errors

d18a215

add wrapper hook to android and update docs. fix justfile command for…

e0f4ecb

… android build

fix kotlin lint errors

12399ac

code cleanup on async node plugin

3d287c4

tmarmer added 8 commits July 22, 2025 14:26

add asyncNodeError hook to ios package

c65d47f

update ios async node test to cause an error instead of throwing one

6e810f6

cleanup error throw in ios test

9ae530e

comment test to fix build temporarily

b820e23

use content structure for onAsyncNodeError and finish ios implementation

1f8b80c

fix ios lint error

dd7bbd8

remove cyclical deps. reduce cache scope of async nodes to individual…

c2209e8

… views

update ios settings in bazelrc to common and async-node.mdx to use ts…

ad2cf3f

… over js

tmarmer force-pushed the async-node-errors branch from 2777574 to ad2cf3f Compare July 22, 2025 18:39

tmarmer and others added 2 commits July 22, 2025 15:43

cleanup swift hooks implementation of async hooks

659c571

Merge branch 'main' into async-node-errors

abd1681

KetanReddy reviewed Jul 25, 2025

View reviewed changes

add view instance to AsyncPluginContext

dc65c54

tmarmer mentioned this pull request Jul 25, 2025

Access player or player state from all plugin types #692

Open

KVSRoyal previously approved these changes Jul 25, 2025

View reviewed changes

KVSRoyal reviewed Jul 25, 2025

View reviewed changes

Comment thread ios/core/Sources/Types/Hooks/Hook.swift Outdated

(ios) change BaseAsyncJSHook to public instead of open

86d1988

tmarmer dismissed KVSRoyal’s stale review via 86d1988 July 25, 2025 20:30

Merge branch 'main' into async-node-errors

4757762

KetanReddy approved these changes Jul 28, 2025

View reviewed changes

KVSRoyal approved these changes Jul 28, 2025

View reviewed changes

brocollie08 approved these changes Jul 28, 2025

View reviewed changes

tmarmer merged commit f46ec23 into main Jul 28, 2025
12 checks passed

tmarmer deleted the async-node-errors branch July 28, 2025 17:51

		public let onAsyncNode: AsyncHook2<JSValue, JSValue>
		public let onAsyncNodeError: Hook2<JSValue, JSValue>


		var count = 0

		let asyncNodePluginPlugin = AsyncNodePluginPlugin()

Conversation

tmarmer commented Jun 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Change Type (required)

Does your PR have any documentation updates?

Release Notes

Uh oh!

tmarmer commented Jun 17, 2025

Uh oh!

codecov Bot commented Jun 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tmarmer commented Jun 24, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmarmer commented Jul 8, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tmarmer commented Jul 9, 2025

Uh oh!

KetanReddy commented Jul 21, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KVSRoyal left a comment

Choose a reason for hiding this comment

Uh oh!

KVSRoyal Jul 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tmarmer commented Jun 17, 2025 •

edited

Loading

codecov Bot commented Jun 17, 2025 •

edited

Loading

KVSRoyal Jul 25, 2025 •

edited

Loading