0no-co / wonka

🎩 A tiny but capable push & pull stream library for TypeScript and Flow
MIT License
709 stars 29 forks source link

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

Closed negezor closed 3 months ago

negezor commented 3 months ago

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 commented 3 months ago

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

negezor commented 3 months ago

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

kitten commented 3 months ago

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