-
Notifications
You must be signed in to change notification settings - Fork 57
Remove async node tracking from core and rely on node-based caching #777
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
base: main
Are you sure you want to change the base?
Conversation
79f5d12 to
5030f72
Compare
| private fun waitForCondition( | ||
| count: Int = 5, | ||
| delay: Long = 500, | ||
| conditions: () -> Boolean = { true }, | ||
| ) { | ||
| var counter = 0 | ||
| while (!conditions() && counter++ < count) runBlockingTest { delay(delay) } | ||
| Assertions.assertTrue(conditions()) | ||
| } |
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.
Changes to the core that let async node changes get batched with other data changes means we need to wait after triggering an update since the view update is no longer triggered directly by calling the async node update function.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #777 +/- ##
==========================================
- Coverage 85.83% 85.80% -0.03%
==========================================
Files 505 505
Lines 22794 22811 +17
Branches 2656 2657 +1
==========================================
+ Hits 19565 19574 +9
- Misses 2900 2908 +8
Partials 329 329 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
plugins/async-node/core/src/index.ts
Outdated
| /** Map of async node id to promises being used to resolve them */ | ||
| inProgressNodes: Set<string>; | ||
| /** node things */ | ||
| nodeThings: Map<string, Node.Node>; |
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.
this definitely needs a better descriptor
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.
Yeah, missed this when cleaning up the code, mb
| ]; | ||
| } | ||
|
|
||
| public markAsChanged(nodes: Set<Node.Node>): void { |
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.
if this is replacing updateAsync, we should make this method more clear on when it's supposed to be used
|
/canary |
|
/canary |
RFC
updatefunction to take in a set of node changes in addition to the existing.updateAsyncfrom theViewin favour of amarkAsChangedin theViewControllerthat allows specific nodes to be marked as updated.markAsChangedwhen an async node is updatedChange Type (required)
Indicate the type of change your pull request is:
patchminormajorN/ADoes your PR have any documentation updates?
Release Notes
markAsChangedfunction to theViewControllerto support triggering updates caused by node changes.AsyncNodePluginandAsyncNodePluginPluginto make use of the above.📦 Published PR as canary version:
0.14.2--canary.777.30429Try this version out locally by upgrading relevant packages to 0.14.2--canary.777.30429