search

microsoft / ChakraCore-wiki

A mirror of the ChakraCore wiki to enable pull requests on the Wiki.
https://github.com/Microsoft/ChakraCore/wiki
Other
22 stars 30 forks source link

Update promise documentation #54

Closed kfarnung closed 6 years ago

kfarnung commented 6 years ago

Needs to wait for https://github.com/Microsoft/ChakraCore/pull/5131 to land. Needs to wait for https://github.com/Microsoft/ChakraCore/pull/5138 to land.

liminzhu commented 6 years ago

Thanks for keeping the docs updated!

Comments on documentation

  1. Consider documenting behavior of JsGetPromiseResult when promise is pending (JsErrorInvalidArgument currently) and rejected (error object). At least the pending behavior isn't very obvious.

I hope it's not too late for comments on those APIs themselves

  1. Most of our enums don't use _. Consider something like JsPromiseStatePending.
  2. This is debatable - I'm not sure about JsGetPromiseResult returning JsErrorInvalidArgument from pending promises. Intuitively, I only expect this if you throw in something other than promise. You can have a valid argument (a promise) but just catch it in a bad state. I'd suggest returning JS_INVALID_REFERENCE if you consider this a valid and successful operation or a new error code like JsErrorPendingPromise for a failed op.
kfarnung commented 6 years ago

@liminzhu I made the documentation changes to resolve your feedback, but it's pending another change to ChakraCore before this can land.

kfarnung commented 6 years ago

Opened https://github.com/Microsoft/ChakraCore/pull/5138

kfarnung commented 6 years ago

@liminzhu @MSLaguana Any other concerns with this documentation?

liminzhu commented 6 years ago

LGTM, thx!