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

Support non-native promises #5689

Merged
merged 1 commit into from May 26, 2016

Conversation

Projects
None yet
2 participants
@cdaringe
Contributor

cdaringe commented May 25, 2016

problem statement

  • non-native promises are not supported

solution

  • support non-native promises

discussion

there is a lot of talk on how-to-detect if something is a promise or not. many people respond to the question with something like "don't try to detect a promise, just Promise.resolve it if you need promise behavior." it's a strange response that doesn't address the intent of question--how to do proper type detection on Promises. it's tricky because a Promise describes both a formal type as well as a behavioral expectation. due to this complication, detecting a promise is not so simple. @zcbenz already denoted that a simple .then check is out of the question, and right fully so. hence, the strategy taken should cover ~99.9% of promise implementations, with the downside that it takes many type checks to reach a conclusion.

closes #5633

@cdaringe cdaringe changed the title from :art: Support non-native promises to Support non-native promises May 25, 2016

@cdaringe

This comment has been minimized.

Contributor

cdaringe commented May 25, 2016

shucks, travis fails. i am still unable to get bootstrap to work fully on OSX and a copy of debian. arg. regardless, this PR is nonetheless very simple. perhaps someone can take a peek and clue me into why the require('../is-promise') isn't properly resolving? can try and debug later too :)

@zcbenz

This comment has been minimized.

Contributor

zcbenz commented May 25, 2016

The is-promise.js needs to be added to filenames.gypi.

@cdaringe

This comment has been minimized.

Contributor

cdaringe commented May 25, 2016

yahoo! thanks @zcbenz

@zcbenz

This comment has been minimized.

Contributor

zcbenz commented May 26, 2016

👍

@zcbenz zcbenz merged commit 8a4b7eb into electron:master May 26, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment