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

util: fix for inspecting promises #3197

Merged
merged 1 commit into from
Oct 6, 2015

Conversation

evanlucas
Copy link
Contributor

The upgrade to v8 4.6 removes ObjectIsPromise. This change utilizes
v8::Value::IsPromise to verify that the argument is indeed a promise.

R= @bnoordhuis, @targos

Ben, not sure if you had another way of doing this, so I went ahead and cherry-picked the changes from #3119. Feel free to close if you want to go a different route though.

@Fishrock123 Fishrock123 added the util Issues and PRs related to the built-in util module. label Oct 6, 2015
@targos
Copy link
Member

targos commented Oct 6, 2015

@evanlucas I rebased the vee-eight-4.6 branch, you can update your PR to keep only the last commit

@bnoordhuis
Copy link
Member

LGTM

@targos
Copy link
Member

targos commented Oct 6, 2015

The upgrade to v8 4.6 removed ObjectIsPromise. This change utilizes
v8::Value::IsPromise to verify that the argument is indeed a promise.

PR-URL: nodejs#3197
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
@evanlucas evanlucas merged commit 4d13dae into nodejs:vee-eight-4.6 Oct 6, 2015
@evanlucas evanlucas deleted the fixpromiseinspect branch October 6, 2015 20:27
@evanlucas
Copy link
Contributor Author

Thanks! Landed in 4d13dae

@bnoordhuis
Copy link
Member

This is probably worth merging back into master to keep the delta small. You have my preemptive LGTM.

@evanlucas
Copy link
Contributor Author

Ok, should I just cherry-pick and push or is there a different way we are doing that?

@bnoordhuis
Copy link
Member

Maybe PR and run CI just in case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants