Skip to content
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

perf: remove extra Promise wrapper for value in fromPromise and toPromise #172

Closed
wants to merge 3 commits into from

Conversation

negezor
Copy link

@negezor negezor commented Jul 15, 2024

Summary

Removes unnecessary Promise wrappers. Since Promise can unroll itself, these operations only create overhead

For example:

Promise.resolve()
  .then(() => Promise.resolve(Promise.resolve(Promise.resolve(Promise.resolve(1)))))
  .then((value) => {
    console.log('val', value)
  });

// val 1

And

new Promise(resolve => resolve(Promise.resolve(Promise.resolve(Promise.resolve(Promise.resolve(1))))))
  .then((value) => console.log('val', value));

// val 1

Set of changes

Removing unnecessary Promise wrappers in sources.ts and sinks.ts.

@JoviDeCroock
Copy link
Member

These changes are not functionally equivalent, the prior code intends to delay the resolution by a micro tick

@negezor
Copy link
Author

negezor commented Jul 15, 2024

But why? This is not used in other parts of the code 🤔 And only non-equivalent one is toPromise.

@kitten
Copy link
Member

kitten commented Jul 15, 2024

This is because promise timing doesn't actually mix very well with Wonka.
This can cause subtle race conditions and bugs. In React Native it even used to cause complete stalls.

The cost here is minimal, in fact, the optimal solution with Wonka is to always avoid converting to or from promises, unless it's absolutely necessary or part of a surfaced API.

The problem with immediately resolving promises is that it becomes very easy to chain logic accidentally into a "single tick" when this isn't desired though. Delaying all promise conversions by a micro tick hence gets rid of a whole class of bugs without causing much of a problem

@kitten kitten closed this Jul 15, 2024
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