improve async node flattening and apply resolver cache to async nodes by tmarmer · Pull Request #694 · player-ui/player

tmarmer · 2025-07-28T19:42:53Z

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

Simplify async node flattening to just check for multi-nodes inside multi-nodes after async nodes are resolved.
Introduce async node id cache in the resolver to allow async nodes to be marked as dirty and limit scope of resolver update to the specific node and use cache for the rest of the update.

📦 Published PR as canary version: 0.12.1--canary.694.25095

Try this version out locally by upgrading relevant packages to 0.12.1--canary.694.25095

codecov · 2025-07-29T15:07:07Z

Codecov Report

❌ Patch coverage is 98.01980% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.63%. Comparing base (a22123b) to head (30a7f51).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...ns/async-node/core/src/utils/traverseAndReplace.ts	86.20%	4 Missing ⚠️
...lugins/async-node/core/src/createAsyncTransform.ts	97.91%	1 Missing ⚠️
plugins/async-node/core/src/index.ts	98.52%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #694      +/-   ##
==========================================
+ Coverage   90.32%   90.63%   +0.30%     
==========================================
  Files         336      341       +5     
  Lines       21258    21409     +151     
  Branches     2128     2173      +45     
==========================================
+ Hits        19202    19404     +202     
+ Misses       2038     1989      -49     
+ Partials       18       16       -2

☔ 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-07-30T16:44:12Z

/canary

brocollie08 · 2025-08-07T19:25:59Z

+### AsyncTransform
+`createAsyncTransform` is a helper function to create the transform for async asset. The function takes a set of options that help adjust the transform generated to your needs:
+- flatten (Optional. Default: true) - Whether or not to flatten chained results of the same asset type into a single container.
+- path (Optional. Default: ["values"]) - The path to the array within the `wrapperAssetType` that will contain the async content.


what's the benefit of giving this as an option?

The path option here is to allow for cases where maybe the warpperAssetType wants its values somewhere else. The best example I can think of would be if someone had something like a table asset with rows instead of values as the path.

brocollie08 · 2025-08-07T19:26:20Z

+- transformAssetType (Required) - The asset type that the transform is matching against.
+- wrapperAssetType (Required) - The asset type that will contain the async content.
+- getNestedAsset (Optional) - Function to get any nested asset that will need to be extracted and kept when creating the wrapper asset. This is useful when combined with `flatten: true` to help chain async content with a single asset type.
+- getAsyncNodeId (Optional) - Function to get the id for the generated async node. By default it uses a format of `async-{asset.id}`


brocollie08 · 2025-08-07T19:31:45Z

+
+  const asyncTransform = (node: Node.ViewOrAsset) => {
+    const id = getAsyncNodeId(node);
+    const asset = getNestedAsset?.(node);


maybe i'm not understanding this properly, the usage of this function is weird. This is entrusting the consumers to pass in a proper getNestedAsset function so we can use it to retrieve the nested asset in our multi-node?

should we not be the one doing this?

The getNestedAsset replaces what the asset argument in the original asyncTransform did. Since we need to repeat it here I decided to use a function. We could alternatively take a nestedAssetPath that defaults to value and get it ourselves I think. Pretty much, this allows for transforms other than our chat-message use case where maybe the chained assets are somewhere else.

tmarmer · 2025-07-31T19:56:42Z

+          return childOptions === undefined
+            ? multiNode
+            : [
+                {
+                  path: [...childOptions.path, childOptions.key],
+                  value: multiNode,
+                },
+              ];


The changes in here allow multi nodes to be parsed if they are the root object. This is to allow async node results to return arrays of data that can either be used to flatten into a higher level multi-node or just to produce a multi node for some property.

tmarmer · 2025-07-31T19:57:58Z

+    asyncChanges?.forEach((id) => {
+      let current: Node.Node | undefined = prevAsyncIdMap.get(id);
+      while (current && prevASTMap.has(current)) {
+        const next = prevASTMap.get(current);
+        if (next && this.resolveCache.has(next)) {
+          this.resolveCache.delete(next);
+        }
+        current = current.parent;
+      }
+    });


This setup here lets us clear some of the cache when an async node updates. I wonder if we can use something similar to the dependency tracker used for data changes here instead?

tmarmer · 2025-07-31T20:03:55Z

+    for (const id of resolvedAST.asyncNodesResolved ?? []) {
+      nextAsyncIdMap.set(id, resolvedAST);


The use of asyncNodesResolved is a special case made necessary by some of the changes to how flattening is handled for async nodes. Because these nodes are resolved by and flattened when the parent node is doing onBeforeResolve, the parent needs to be aware of the async ids that were resolved so it can have its cache cleared if that node updates.

tmarmer · 2025-07-31T20:06:51Z

-        .filter((index) => index !== -1);
-
-      const newValues = resolvedAST.values.map((mValue) => {
+      resolvedAST.values = resolvedAST.values.flatMap((mValue) => {


The changes here and below are just removing some of the flattening work since it's handled by the async node plugin now.

tmarmer · 2025-07-31T20:07:43Z

+  public updateAsync(asyncNode: string): void {
+    const update = this.resolver?.update(new Set(), new Set([asyncNode]));


Can't used undefined for the data changes since that clears the whole cache.

tmarmer · 2025-08-07T20:38:21Z

+
+  const asyncTransform = (node: Node.ViewOrAsset) => {
+    const id = getAsyncNodeId(node);
+    const asset = getNestedAsset?.(node);


The getNestedAsset replaces what the asset argument in the original asyncTransform did. Since we need to repeat it here I decided to use a function. We could alternatively take a nestedAssetPath that defaults to value and get it ourselves I think. Pretty much, this allows for transforms other than our chat-message use case where maybe the chained assets are somewhere else.

tmarmer · 2025-08-07T20:39:35Z

+### AsyncTransform
+`createAsyncTransform` is a helper function to create the transform for async asset. The function takes a set of options that help adjust the transform generated to your needs:
+- flatten (Optional. Default: true) - Whether or not to flatten chained results of the same asset type into a single container.
+- path (Optional. Default: ["values"]) - The path to the array within the `wrapperAssetType` that will contain the async content.


The path option here is to allow for cases where maybe the warpperAssetType wants its values somewhere else. The best example I can think of would be if someone had something like a table asset with rows instead of values as the path.

KetanReddy · 2025-08-13T19:16:04Z

  wrapperAssetType,
  asset,
-  flatten,
+  flatten = true,


is this backwards compatible?

Oops, no, I meant to undo this after moving stuff to the createAsyncTransform. Good catch, will remove.

…work

…o include node extraction logic for async nodes

…lugin implementation

…or better flattening

tmarmer commented Jul 28, 2025

View reviewed changes

Comment thread plugins/async-node/core/src/transform.ts Outdated

tmarmer force-pushed the async-node-flattening branch from 33236d4 to c45c574 Compare July 29, 2025 14:36

tmarmer force-pushed the async-node-flattening branch from 331debd to 6cd630d Compare July 30, 2025 16:20

tmarmer force-pushed the async-node-flattening branch from 6551459 to c3bb8d1 Compare August 6, 2025 18:26

tmarmer marked this pull request as ready for review August 6, 2025 18:26

tmarmer requested a review from a team as a code owner August 6, 2025 18:26

brocollie08 reviewed Aug 7, 2025

View reviewed changes

tmarmer commented Aug 7, 2025

View reviewed changes

KetanReddy added the minor Increment the minor version when merged label Aug 13, 2025

KetanReddy reviewed Aug 13, 2025

View reviewed changes

tmarmer added 18 commits August 14, 2025 13:46

implement initial pass of flattening fixes. Left todos for remaining …

d5bb466

…work

move parsing code from async node plugin to parser. update resolver t…

761aeb0

…o include node extraction logic for async nodes

add initial caching strategy for async nodes

4babbdd

add bench tests for async transform and add comments

905e152

move some weird async transform code into its own function

dfc04c3

code cleanup after rebase

6ba9a07

fix build issues

f386b59

more cleanup

4562b65

fix linting issues in resolver

30f383f

separate transform bench tests into their own file

03a4189

write more tests. remove parseMultiNode and improve multi node view p…

805edd1

…lugin implementation

remove unused arg from async transform

9fd78db

add resolver and view tests

07768c7

set flatMap to just map in resolver

7fbd8ff

revert changes to async transform. add createAsyncTransform utility f…

01f4b34

…or better flattening

add more tests for extractNodeFromPath

125992e

update test names in createAsyncTransform.test.ts

ebfebc2

undo default flatten value for async transform

9a9ee75

tmarmer force-pushed the async-node-flattening branch from 185649d to 9a9ee75 Compare August 14, 2025 18:19

KetanReddy previously approved these changes Aug 14, 2025

View reviewed changes

tmarmer added 2 commits August 14, 2025 15:03

Merge branch 'main' into async-node-flattening

5e96b60

preserve existing async nodes resolved array in AsyncNodePluginPlugin

30a7f51

tmarmer dismissed KetanReddy’s stale review via 30a7f51 August 15, 2025 15:23

KetanReddy approved these changes Aug 15, 2025

View reviewed changes

tmarmer merged commit 14177bb into main Aug 15, 2025
12 checks passed

tmarmer deleted the async-node-flattening branch August 15, 2025 16:03

		for (const id of resolvedAST.asyncNodesResolved ?? []) {
		nextAsyncIdMap.set(id, resolvedAST);

		public updateAsync(asyncNode: string): void {
		const update = this.resolver?.update(new Set(), new Set([asyncNode]));

Conversation

tmarmer commented Jul 28, 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!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

tmarmer commented Jul 30, 2025

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tmarmer commented Jul 28, 2025 •

edited

Loading

codecov Bot commented Jul 29, 2025 •

edited

Loading