gobwas / broody-promises

Broody implementation of Promise A+.
0 stars 2 forks source link

Suggest Bluebird Instead? #1

Closed dchambers closed 9 years ago

dchambers commented 9 years ago

It maybe useful to re-direct people to Bluebird as that library now also supports synchronous-inspection, plus when I tried swapping in your promise library to an existing code base I found that it errored, and so it looks like there may be some bugs there.

gobwas commented 9 years ago

Hi!

Which bugs did you caught? There was some fixes few month ago.

Bluebird is a great library, and Im using it on some projects, but, it a much more bigger than Broodies (at now, at broody-promises@0.3.0, without lodash dep, its about ~1.5KB gzipped vs ~11KB bluebird).

dchambers commented 9 years ago

Hi @gobwas, I used the latest version on NPM at the time (0.2.6) but I see that you've just now updated it to 0.3.1 (:+1:) , so I'll give that a try when I get back from lunch...

gobwas commented 9 years ago

Yes, released the lightweight version without lodash :)

dchambers commented 9 years ago

Hi @gobwas, I just tried again with 0.3.1 and it still errors — an assertion fails within SystemJS when I pair it with broody-promises, but broody-promises itself does not error. Unfortunately, as this failure occurs as part of a large application, I don't even have a simple test case I can share with you.

However, I notice that you've written your own tests rather than using the Promises/A+ Compliance Test Suite to test this library with, and so I imagine you'd get very good feedback about exactly which edge case you aren't supporting by using that test suite too.

gobwas commented 9 years ago

Yes, @dchambers, thank you for your link - I will use this test as soon, as possible =)

dchambers commented 9 years ago

:+1:

dchambers commented 9 years ago

Hi @gobwas, I see that you hooked up the Promises/A+ Compliance Test Suite, and that there were some test failures. Have you had a chance to analyse any of these yet? Do you know if broody-promises is fundamentally at odds with Promises/A+ compliance as it currently stands?

By the way, I found that Bluebird's synchronous-inspection feature only works out-of-the-box if you haven't got any then() chains, and I had to write some code to manually trigger the chain of outstanding promises myself for promises when then() was used. It's not clear to me why this wasn't done automatically within Bluebirds's value() method, but it looks like by delaying then() invocations, but still performing them synchronously, Bluebird is able to resolve promises synchronously while seemingly (I think) still being Promise A+ compatible.

gobwas commented 9 years ago

Hi @dchambers! Yep, I've hooked this up, but sadly can not fix failures yet =( But I will. I think there are no fundamental differences.

dchambers commented 9 years ago

Okay, sounds good :+1: Hope it's not too painful...

gobwas commented 9 years ago

Hey @dchambers!

I've found that the spec(3.1) says, that all functions passed into then method must be called asynchronously, so it means, that if Broodys will pass the spec test, they will have the same as Bluebird behavior.

:beers: :laughing:

dchambers commented 9 years ago

@gobwas, good work figuring it out!

This makes sense, and agrees with everything I've learned recently from the Bluebird guys. It seems that Bluebird's synchronous-inspection feature (using value()) only works for promises that didn't use then(), since that would require synchronous-resolution, which goes against the spirit of the spec.

I tried to encourage them to optionally allow synchronous-resolution too (e.g. using resolveSynchronously()), since this could be done safely provided the developer ensures that any code following the resolveSynchronously() invocation won't be adversely affected by the early resolution of any of the preceding promises.

The problem with always resolving then() synchronously of course is that some code may be adversely affected by this (like SystemJS was), so it must not be done by default, and can only safely be done by the developer in a controlled manner.

gobwas commented 9 years ago

So now, at 0.4.0 broodies accepts an options object, with ability to switch it in sync-resolving mode. But, anyway, it already in sync mode by default, cause broodies was designed directly for this purpose. Enjoy!

dchambers commented 9 years ago

Thanks so much, will give this a go shortly :+1:

gobwas commented 9 years ago

:beers:

dchambers commented 9 years ago

Hi @gobwas,

I've tried to have a play with the new release of broody-promises, but it isn't actually usable for my use case because it's not possible to centrally control whether promises are resolved synchronously, and since it forces each promise within the code to either always be resolved synchronously or always be resolved asynchronously, whereas it's actually only safe to synchronously resolve a promise if you happen to know it won't adversely affect the remaining code executed within the same event-tick.

This is an issue because:

  1. The code creating the promises is a third-party library in my case (SystemJS), so I can't easily modify the source.
  2. Whether it's safe to synchronously resolve a promise provided for an app by a library, depends on what the app does next, which is outside of the library's control.
  3. In the case of SystemJS, the library itself isn't tolerant of synchronously resolved promises.

As per the spec, for a library to be Promises/A+ compliant it must resolve promises within then() methods asynchronously. This only leaves a couple of options:

  1. You make value() synchronously resolve any outstanding promises, and make it the developer's responsibility not to invoke value() in cases where it could be a problem.
  2. You use value() for synchronous-inspection only (like Bluebird does), but you have a companion method (say syncResolve(), force() or resolveSynchronously()) that allows the developer to force synchronous-resolution if that's what they want, so that value() then has access to resolved value.

I would have originally argued for number 1, but the Bluebird guys made me realize that value() is always safe to do, even for library code, and can simplify promises code, whereas syncResolve() or force() are potentially dangerous, and only ever safe for an app to invoke.

gobwas commented 9 years ago

Hi @dchambers!

Im sorry, but Ive read your post twice, and can not realize, what behavior do u want from BP? ) Could you show it in some code snippet?

gobwas commented 9 years ago

Any way, I've updated to 0.5.0, where is no default sync mode, but Promise.sync wrapped instead. Check it @dchambers =)