util: correctly inspect Map/Set Iterators by evanlucas · Pull Request #3119 · nodejs/node · GitHub

Currently, inspecting new Map().keys() or other iterators will return an empty object.

I am opening more for discussion of how we want (if at all) to address not being able to inspect map and set iterators. I first had first attempting using Debug.MakeMirror but the performance of doing it this way was about 10x better.

Related: #3107

This will consume iterator entirely and make it useless

For Maps and Sets?

If you print an iterator, python just prints the type of iterator. May be we should do the same. Otherwise we'll exhaust the iterator as vkurchatkin pointed out.

Ah yea, I see what you mean.

So I guess that means that we will have to use Debug.MakeMirror and fetch the values?

Yes.

Style nit. Or operator should be in the previous line.

@thefourtheye Do we have an eslint rule for that, btw?

make lint passes with both. Be happy to change if that is the preferred way though

Doesn't it fit on one line? It works out to exactly 80 characters, doesn't it?

Updated to use Debug.MakeMirror. It also takes advantage of using IteratorMirror.preview which does not change the backing iterator state.

Can you rebase and use ensureDebugIsInitialized() here?

Updated to utilize v8::Value::IsMapIterator and v8::Value::IsSetIterator

Can you name this binding? That's more in line with other source files.

Mostly LGTM. I think the constructor.name === 'Array' check should come with regression tests of its own.

Alright, broke out the constructor.name === 'Array' check into it's own commit with a test. Is that single test sufficient or should I add more? Honestly, the only other way I've been able to reproduce it is from the map/set iterators

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

LGTM

In the event an Array is created in a Debug context, the constructor will be Array, but !== Array. This adds a check constructor.name === 'Array' to handle edge cases like that. PR-URL: nodejs#3119 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Previously, a MapIterator or SetIterator would not be inspected properly. This change makes it possible to inspect them by creating a Debug Mirror and previewing the iterators to not consume the actual iterator that we are trying to inspect. This change also adds a node_util binding that uses v8's Value::IsSetIterator and Value::IsMapIterator to verify that the values passed in are actual iterators. Fixes: nodejs#3107 PR-URL: nodejs#3119 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Thanks! Landed in 089d688 and 8853388

In the event an Array is created in a Debug context, the constructor will be Array, but !== Array. This adds a check constructor.name === 'Array' to handle edge cases like that. PR-URL: #3119 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Previously, a MapIterator or SetIterator would not be inspected properly. This change makes it possible to inspect them by creating a Debug Mirror and previewing the iterators to not consume the actual iterator that we are trying to inspect. This change also adds a node_util binding that uses v8's Value::IsSetIterator and Value::IsMapIterator to verify that the values passed in are actual iterators. Fixes: #3107 PR-URL: #3119 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Fishrock123 added the util Issues and PRs related to the built-in util module. label Sep 29, 2015

vkurchatkin reviewed Sep 29, 2015
View reviewed changes

evanlucas force-pushed the iterators branch from ed5c500 to 04b8de3 Compare September 30, 2015 03:03

thefourtheye reviewed Sep 30, 2015
View reviewed changes

evanlucas force-pushed the iterators branch from 04b8de3 to 7da3f9a Compare September 30, 2015 03:10

bnoordhuis reviewed Sep 30, 2015
View reviewed changes

targos mentioned this pull request Sep 30, 2015

Speed up promise introspection #3130

Merged

evanlucas force-pushed the iterators branch from 7da3f9a to 8204269 Compare September 30, 2015 12:49

bnoordhuis reviewed Oct 1, 2015
View reviewed changes

evanlucas force-pushed the iterators branch from 8204269 to 2b8c663 Compare October 1, 2015 20:25

evanlucas mentioned this pull request Oct 5, 2015

Time to play with V8 4.6 #2688

Closed

bnoordhuis reviewed Oct 5, 2015
View reviewed changes

evanlucas force-pushed the iterators branch 3 times, most recently from 66169e3 to c7d7eab Compare October 5, 2015 21:38

evanlucas force-pushed the iterators branch from c7d7eab to 0541eee Compare October 6, 2015 01:23

evanlucas added 2 commits October 5, 2015 20:24

util: fix check for Array constructor

089d688

In the event an Array is created in a Debug context, the constructor will be Array, but !== Array. This adds a check constructor.name === 'Array' to handle edge cases like that. PR-URL: nodejs#3119 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

evanlucas force-pushed the iterators branch from 0541eee to 8853388 Compare October 6, 2015 01:28

evanlucas closed this Oct 6, 2015

evanlucas deleted the iterators branch October 6, 2015 01:28

evanlucas merged commit 8853388 into nodejs:master Oct 6, 2015

evanlucas mentioned this pull request Oct 6, 2015

util: fix for inspecting promises #3197

Merged

jasnell mentioned this pull request Oct 8, 2015

Preparing v4.x for LTS #3252

Closed

29 tasks

jasnell added the lts-landed-on-v4.x label Oct 8, 2015

jasnell mentioned this pull request Oct 8, 2015

Release proposal: v4.2.0 'Argon' (LTS) #3258

Closed

util: correctly inspect Map/Set Iterators by evanlucas · Pull Request #3119 · nodejs/node · GitHub

Uh oh!

Uh oh!

util: correctly inspect Map/Set Iterators - #3119

util: correctly inspect Map/Set Iterators#3119evanlucas merged 2 commits intonodejs:masternodejs/node:masterfrom evanlucas:iteratorsevanlucas/node:iteratorsCopy head branch name to clipboard

Conversation

evanlucas commented Sep 29, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evanlucas commented Sep 30, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evanlucas commented Oct 5, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented Oct 5, 2015

Uh oh!

evanlucas commented Oct 5, 2015

Uh oh!

evanlucas commented Oct 5, 2015

Uh oh!

bnoordhuis commented Oct 5, 2015

Uh oh!

evanlucas commented Oct 6, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

util: correctly inspect Map/Set Iterators#3119
evanlucas merged 2 commits into
nodejs:masterfrom
evanlucas:iterators