Skip to content

Add wrapResolved to be a faster version of wrapPromise#103

Merged
platypii merged 1 commit intomasterfrom
wrapResolved
Apr 4, 2025
Merged

Add wrapResolved to be a faster version of wrapPromise#103
platypii merged 1 commit intomasterfrom
wrapResolved

Conversation

@platypii
Copy link
Copy Markdown
Contributor

@platypii platypii commented Apr 3, 2025

This is a fix for the issue with arrayDataFrame flickering.

The problem was that it's using wrapPromise. wrapPromise effectively uses Promise.resolved(value).then() which means that data has to wait for a microtask to be scheduled on the next event loop tick.

I added a new function wrapResolved which expects a concrete value, and returns a WrappedPromise but without adding a callback delay. This makes arrayDataFrame and anything using it faster. Without this change, scrolling would flicker unless wrapped in a rowCache (which really shouldn't be necessary for an array-backed dataframe).

I also changed the signature of wrapPromise from Promise<T> | T to just Promise<T>. This force downstream users to make a choice between wrapPromise and wrapResolved. Users should always choose the faster one, if they can. Should be considered a breaking api change (0.14.0).

@platypii platypii requested review from bleakley and severo April 3, 2025 21:49
Copy link
Copy Markdown
Contributor

@severo severo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea.

An alternative would have been to keep the conditional (if (!(promise instanceof Promise)) {) in wrapPromise and just synchronously set .resolve = value and return.

But I like the clarity of one function doing one thing, so I prefer the code now.

@platypii platypii merged commit 865e7a2 into master Apr 4, 2025
8 checks passed
@platypii platypii deleted the wrapResolved branch April 4, 2025 16:37
@bleakley bleakley mentioned this pull request Apr 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants