Ability to remove a resolved async node #404

sakuntala-motukuri · 2024-06-28T21:37:37Z

Ability to remove a resolved async node

Change Type (required)

Attaching logs FYR to see how the fucntionality works :

Key Changes

This version of the plugin allows for the flexibility needed to handle complex async updates, including conditionally removing resolved async nodes from rendering.

Test results:
https://app.buildbuddy.io/invocation/fc720b64-f848-4123-9e95-22216f410acc

patch
minor
major

Does your PR have any documentation updates?

Updated docs
No Update needed
Unable to update docs

Version

Published prerelease version: 0.8.0-next.4

Changelog

Release Notes

JS Package Cleanup (#442)

Fix migration issues in JS packages

Does your PR have any documentation updates?

Updated docs
No Update needed
Unable to update docs

Bazel 6 Migration (#252)

Swaps the repo internals to use bazel@6, rules_js, bazel modules, vitest and tsup for the core + plugin builds

🚀 Enhancement

Bazel 6 Migration #252 (@adierkens @hborawski @brocollie08 @KetanReddy @sugarmanz [email protected] @nancywu1 @mercillo @mrigankmg @sakuntala-motukuri)

🐛 Bug Fix

Ability to remove a resolved async node #404 (@sakuntala-motukuri @brocollie08)
ReactAsset improve error messages #444 (@cehan-Chloe)
JS Package Cleanup #442 (@KetanReddy)
Specific Temurin Version #445 (@brocollie08)
iOS: Canary release support + SPM automated releases #434 (@hborawski)
apply strategy from pr builds to main & release builds #440 (@sugarmanz)
Add dsl component to async node #439 (@sakuntala-motukuri)
Bazel clean up #435 (@brocollie08)
[JVM] Publish next releases to release repository (fixed versions) #428 (@sugarmanz)
This PR enables to add footer component to info reference asset in android #411 (@sakuntala-motukuri)
Fix NPM Registry URL #427 (@KetanReddy)

🏠 Internal

Remove alternate binding parsers from bundle #432 (@adierkens)
Revert "[JVM] Publish next releases to release repository" #429 (@sugarmanz)
[JVM] Publish next releases to release repository #426 (@sugarmanz)

📝 Documentation

Update CONTRIBUTING.md #433 (@brocollie08)

Authors: 11

@brocollie08
@nancywu1
Adam Dierkens (@adierkens)
Chloe (@cehan-Chloe)
Harris Borawski (@hborawski)
Jeremiah Zucker (@sugarmanz)
Ketan Reddy (@KetanReddy)
Marlon "Marky" Ercillo (@mercillo)
Mrigank Mehta (@mrigankmg)
nancywu1 ([email protected])
Niharika Motukuri (@sakuntala-motukuri)

codecov · 2024-07-09T19:25:51Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.84%. Comparing base (082b556) to head (44f5bca).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #404      +/-   ##
==========================================
+ Coverage   91.72%   91.84%   +0.11%     
==========================================
  Files         338      338              
  Lines       26824    26831       +7     
  Branches     1941     1941              
==========================================
+ Hits        24605    24642      +37     
+ Misses       2206     2175      -31     
- Partials       13       14       +1

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

plugins/async-node/core/src/index.test.ts

brocollie08 · 2024-07-12T18:11:13Z

plugins/async-node/core/src/index.test.ts

+
+  expect(view?.actions[0].asset.type).toBe("action");
+  expect(view?.actions[1].asset.type).toBe("action");
+  expect(view?.actions[2]).toBeUndefined();


is this not checking for something out of bounds?

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

I mean I think we shouldn't check for view?.actions[2] at all because there's no significance to it

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

sakuntala-motukuri · 2024-07-16T06:59:56Z

Updated business logic

plugins/async-node/core/src/index.ts

plugins/async-node/core/src/index.test.ts

sugarmanz · 2024-07-18T19:43:06Z

plugins/async-node/core/src/index.test.ts

+  await new Promise((resolve) => setTimeout(resolve, 100));
+
+  // Verify that the updateNumber is still 0, indicating that the async node was not resolved
+  expect(updateNumber).toBe(0);


Hmm, I'm a bit skeptical with this test -- it doesn't look like you have the setup to update updateNumber on view updates. And even then, I would expect it to update at least once for when the initial view resolution occurs when you start the Player

yes I removed it on purpose , as I wanted to check if the view would still be resolved even if the baseplugin instance doesn't exist , do we still want the view to update ?

plugins/async-node/core/src/index.test.ts

brocollie08

We have duplicate code in applyResolverHooks, we can probably clean that up to be called in the apply method. We should probably follow the convention of ApplicabilityPlugin and rename it to applyResolver

brocollie08 · 2024-07-19T03:19:17Z

plugins/async-node/core/src/index.test.ts

+
+  expect(view?.actions[0].asset.type).toBe("action");
+  expect(view?.actions[1].asset.type).toBe("action");
+  expect(view?.actions[2]).toBeUndefined();


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

sugarmanz · 2024-07-19T20:41:51Z

docs/site/pages/plugins/async-node.mdx

@@ -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:


I think this should stay AsyncNodePlugin

sugarmanz · 2024-07-19T20:43:43Z

docs/site/pages/plugins/async-node.mdx

@@ -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


Suggested change

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

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 behavior 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.

brocollie08 · 2024-07-22T12:13:44Z

docs/site/pages/plugins/async-node.mdx


+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.


brocollie08 · 2024-07-22T12:14:40Z

plugins/async-node/core/src/index.test.ts

+
+  expect(view?.actions[0].asset.type).toBe("action");
+  expect(view?.actions[1].asset.type).toBe("action");
+  expect(view?.actions[2]).toBeUndefined();


I mean I think we shouldn't check for view?.actions[2] at all because there's no significance to it

brocollie08 · 2024-07-22T22:06:15Z

plugins/async-node/core/src/index.ts

-
-        return newNode;
-      });
+      this.handleAsyncApi(resolver, view);


Suggested change

this.handleAsyncApi(resolver, view);

this.applyResolver.bind(this)

to keep convention consistent with other ViewPlugins

…ng , fixed this issue now

sakuntala-motukuri changed the base branch from main to bazel-6 June 28, 2024 21:37

sakuntala-motukuri requested a review from sugarmanz June 28, 2024 21:38

sakuntala-motukuri force-pushed the bazel-6-Ability-to-remove-a-resolved-async-node branch from d44a01b to 5fe2d2a Compare July 8, 2024 16:10

sakuntala-motukuri changed the base branch from bazel-6 to bazel-6-android July 8, 2024 16:12

sakuntala-motukuri force-pushed the bazel-6-Ability-to-remove-a-resolved-async-node branch from af27395 to bed39af Compare July 9, 2024 18:56

Base automatically changed from bazel-6-android to bazel-6 July 9, 2024 21:12

sugarmanz reviewed Jul 10, 2024

View reviewed changes

plugins/async-node/core/src/index.test.ts Outdated Show resolved Hide resolved

sugarmanz reviewed Jul 10, 2024

View reviewed changes

plugins/async-node/core/src/index.test.ts Outdated Show resolved Hide resolved

sakuntala-motukuri force-pushed the bazel-6-Ability-to-remove-a-resolved-async-node branch from 3319bbe to f5731a4 Compare July 11, 2024 19:25

sakuntala-motukuri requested a review from sugarmanz July 11, 2024 19:26

brocollie08 reviewed Jul 12, 2024

View reviewed changes

Rebased onto main branch for ability to remove a resolved async node

50591e0

sakuntala-motukuri force-pushed the bazel-6-Ability-to-remove-a-resolved-async-node branch from f5731a4 to 50591e0 Compare July 16, 2024 06:54

sakuntala-motukuri changed the base branch from bazel-6 to main July 16, 2024 06:55

Merge branch 'main' into bazel-6-Ability-to-remove-a-resolved-async-node

12f9d46

sakuntala-motukuri changed the title ~~Bazel 6 ability to remove a resolved async node~~ Ability to remove a resolved async node Jul 16, 2024

brocollie08 reviewed Jul 16, 2024

View reviewed changes

plugins/async-node/core/src/index.ts Outdated Show resolved Hide resolved

sakuntala-motukuri added 2 commits July 17, 2024 23:30

Updated code with callback mechanism and updated respective test cases

9e27f2d

Merge branch 'main' into bazel-6-Ability-to-remove-a-resolved-async-node

ac7fec1

sugarmanz reviewed Jul 18, 2024

View reviewed changes

Fixed PR review comments

a198f1f

sugarmanz reviewed Jul 18, 2024

View reviewed changes

plugins/async-node/core/src/index.test.ts Outdated Show resolved Hide resolved

Fixed PR review comments second time

fc73d00

sugarmanz reviewed Jul 18, 2024

View reviewed changes

plugins/async-node/core/src/index.test.ts Outdated Show resolved Hide resolved

updated test to be in a waitfor

b95b53d

sakuntala-motukuri requested a review from brocollie08 July 18, 2024 22:04

removed AsyncNodePlugin initialisation to satiate the test accordingly

49a4d1d

brocollie08 reviewed Jul 19, 2024

View reviewed changes

Updated docs for async-node.mdx

6764f0d

sugarmanz reviewed Jul 19, 2024

View reviewed changes

Updated suggested changes

43e0edb

brocollie08 reviewed Jul 22, 2024

View reviewed changes

sakuntala-motukuri added 3 commits July 22, 2024 14:11

Fixed additional review comments

0030479

Merge branch 'main' into bazel-6-Ability-to-remove-a-resolved-async-node

5c556c9

Code cleanup as per ask by Tony

3bb57e1

sakuntala-motukuri requested a review from brocollie08 July 22, 2024 21:29

Merge branch 'main' into bazel-6-Ability-to-remove-a-resolved-async-node

c0d9dff

sakuntala-motukuri requested a review from sugarmanz July 22, 2024 21:40

brocollie08 reviewed Jul 22, 2024

View reviewed changes

Updated naming convention

8403522

sakuntala-motukuri requested a review from brocollie08 July 22, 2024 22:11

sakuntala-motukuri and others added 3 commits July 23, 2024 13:26

view instance was resolving to null due to which the tests were faili…

6cd0a7f

…ng , fixed this issue now

Merge branch 'main' into bazel-6-Ability-to-remove-a-resolved-async-node

428a217

bind

44f5bca

brocollie08 added the patch Increment the patch version when merged label Jul 24, 2024

brocollie08 approved these changes Jul 24, 2024

View reviewed changes

brocollie08 marked this pull request as ready for review July 24, 2024 03:38

brocollie08 requested review from KetanReddy and adierkens as code owners July 24, 2024 03:38

brocollie08 merged commit d80fdac into main Jul 24, 2024
11 checks passed

brocollie08 deleted the bazel-6-Ability-to-remove-a-resolved-async-node branch July 24, 2024 03:39

This was referenced Jul 29, 2024

View Parser Plugin Decomposition #413

Merged

Fixed switch parsing syntax issue for dynamic switches #458

Merged

Fix J2V8 Debugger #443

Merged

Update rules1.1.2 #473

Merged

Docs and Release Fixes #474

Merged

Docsite Fixes #475

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ability to remove a resolved async node #404

Ability to remove a resolved async node #404

sakuntala-motukuri commented Jun 28, 2024 •

edited by intuit-svc

Loading

codecov bot commented Jul 9, 2024 •

edited

Loading

brocollie08 Jul 12, 2024

brocollie08 Jul 19, 2024

brocollie08 Jul 22, 2024

sakuntala-motukuri Jul 22, 2024

sakuntala-motukuri commented Jul 16, 2024

sugarmanz Jul 18, 2024

sakuntala-motukuri Jul 18, 2024

brocollie08 left a comment

brocollie08 Jul 19, 2024

sugarmanz Jul 19, 2024

sakuntala-motukuri Jul 19, 2024

sugarmanz Jul 19, 2024

sakuntala-motukuri Jul 19, 2024

brocollie08 Jul 22, 2024

brocollie08 Jul 22, 2024

brocollie08 Jul 22, 2024


		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.

	this.handleAsyncApi(resolver, view);
	this.applyResolver.bind(this)

Ability to remove a resolved async node #404

Ability to remove a resolved async node #404

Conversation

sakuntala-motukuri commented Jun 28, 2024 • edited by intuit-svc Loading

Change Type (required)

Does your PR have any documentation updates?

Version

Release Notes

JS Package Cleanup (#442)

Does your PR have any documentation updates?

Bazel 6 Migration (#252)

🚀 Enhancement

🐛 Bug Fix

🏠 Internal

📝 Documentation

Authors: 11

codecov bot commented Jul 9, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sakuntala-motukuri commented Jul 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brocollie08 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sakuntala-motukuri commented Jun 28, 2024 •

edited by intuit-svc

Loading

codecov bot commented Jul 9, 2024 •

edited

Loading