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.

Sign up for GitHub

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

Jump to bottom

util: use V8 C++ API for inspecting Promises #12254

Merged
merged 1 commit into from
Apr 8, 2017
Merged

util: use V8 C++ API for inspecting Promises #12254

merged 1 commit into from
Apr 8, 2017

Conversation

TimothyGu
Copy link
Member

@TimothyGu TimothyGu commented Apr 6, 2017 •
edited
Loading

Avoid using the deprecated debug mirror API.

Refs: #11875
Refs: #12243

CI: https://ci.nodejs.org/job/node-test-pull-request/7244/

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

url

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. util Issues and PRs related to the built-in util module. labels Apr 6, 2017
@mscdex mscdex added the promises Issues and PRs related to ECMAScript promises. label Apr 6, 2017
addaleax
addaleax approved these changes Apr 6, 2017
View reviewed changes
lib/util.js
var str = formatValue(ctx, internals.value, nextRecurseTimes);
if (internals.status === 'rejected') {
var str = formatValue(ctx, result, nextRecurseTimes);
if (state === binding.kRejected) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also fyi @Fishrock123, this seems like a cleaner way to inspect Promise states than what the abort-on-rejected PR currently does…

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Errr, this allows you to get the actual result object e.g. an error without going through the Debug API? I was told that was not possible to do synchronously.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Fishrock123 this set of APIs is relatively new, having been added in V8 5.7 in v8/v8@2843258. That might be why.

joshgav
joshgav reviewed Apr 7, 2017
View reviewed changes
Copy link
Contributor

@joshgav joshgav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but perhaps returning an object instead of an array would be better.

src/node_util.cc
auto isolate = args.GetIsolate();

Local<Promise> promise = args[0].As<Promise>();
Local<Array> ret = Array::New(isolate, 2);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might it be better to use Object and name the properties? might be easier if we want to extend with additional metadata, and would allow the JavaScript to be de-coupled from the order of items in the array.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides the fact that this is simpler to implement, it is also the approach taken by the existing GetProxyDetails. Plus, I don't think it is very likely for promises to change its internal properties anytime soon.

@TimothyGu TimothyGu mentioned this pull request Apr 7, 2017
Closed
4 tasks
@TimothyGu
util: use V8 C++ API for inspecting Promises
a37273c 
PR-URL: nodejs#12254
Refs: nodejs#11875
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Josh Gavant <josh.gavant@outlook.com>
@TimothyGu
Copy link
Member Author

TimothyGu commented Apr 8, 2017 •
edited
Loading

Rebased CI: https://ci.nodejs.org/job/node-test-pull-request/7285/ https://ci.nodejs.org/job/node-test-pull-request/7286/

@TimothyGu TimothyGu merged commit a37273c into nodejs:master Apr 8, 2017
@TimothyGu TimothyGu deleted the promise-inspect branch April 8, 2017 18:54
@TimothyGu
Copy link
Member Author

Landed in a37273c.

@italoacasas
Copy link
Contributor

cc @TimothyGu

@addaleax
Copy link
Member

This depends on V8 5.7 iiuc, so I’m adding the dont-land labels here

@jasnell jasnell mentioned this pull request May 11, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joshgav joshgav joshgav left review comments

@Fishrock123 Fishrock123 Fishrock123 left review comments

@cjihrig cjihrig cjihrig approved these changes

@addaleax addaleax addaleax approved these changes

@evanlucas evanlucas evanlucas approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. promises Issues and PRs related to ECMAScript promises. util Issues and PRs related to the built-in util module.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@TimothyGu @italoacasas @addaleax @bnoordhuis @evanlucas @Fishrock123 @cjihrig @joshgav @mscdex @nodejs-github-bot