util: improve Weak(Map|Set) support #19259

BridgeAR · 2018-03-09T16:22:17Z

This improves our Weak(Map|Set) support significantly.

commit: util: improve iterator inspect output

1) So far extra keys on an (Set|Map)Iterator were ignored. Those
   will now be visible.
2) Improve the performance of showing (Set|Map)Iterator by using
   the cloned iterator instead of copying all entries first.
3) So far the output was strictly limited to up to 100 entries.
   The limit will now depend on `maxArrayLength` instead (that
   default is set to 100 as well) and the output indicates that
   more entries exist than visible.

commit: util: show Weak(Set|Map) entries in inspect (Fixes: Console log WeakSet and WeakMap always empty #19001):

This adds support for WeakMap and WeakSet entries in `util.inspect`.
The output is limited to a maximum entry length of `maxArrayLength`.

3. commit: `util,assert: improve Weak(Map|Set) support` (Fixes: #18227):
This improves support for Weak(Map|Set) in `assert.deepStrictEqual`, `assert.notDeepStrictEqual` and `util.isDeepStrictEqual`. Now, the entries of a WeakMap / WeakSet will also be compared instead of being ignored as before.

I would like to rework the maxArrayLength and used that as limiting factor. I would like to get opinions about that.

I am not certain what this should be when it comes to semver. For me it feels like a semver-minor. Other opinions?

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

BridgeAR · 2018-03-09T16:23:07Z

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

TimothyGu

Strong 👎 on being able to introspect WeakMaps and WeakSets, especially in assert, since the behavior of weak collections depends on GC and it is not good developer experience to asserting on something whose result is not guaranteed/deterministic.

I’m unhappy with adding support to util, but I can see the value of doing so.

BridgeAR · 2018-03-09T16:36:15Z

@TimothyGu I am aware that this is GC dependend but from the former discussion this was a option that we did not discuss so far and to me it felt like the best option possible.

When choosing between any alternatives and this, I think this is the best way to handle it.

BridgeAR · 2018-03-09T16:39:06Z

I can add a warning next to it: when using assert and WeakMap and WeakSet please be aware that there is no guarantee that the comparison works as expected if there is no reference left to any of the entries.

joyeecheung · 2018-03-10T05:44:39Z

I can see the value of adding it to until.inspect for debugging purposes, but -1 to adding assert support as well.

devsnek · 2018-03-10T06:07:52Z

-1 to assert, +1 to util.inspect

@hashseed do you know if there are any plans to add the apis we have in internal/v8 like SetIterator::New(isolate, args[0].As<SetIterator>()->Map()) for SetIteratorClone, WeakMap::Entries etc

BridgeAR · 2018-03-11T00:56:49Z

Ok, I am going to remove the assert part for now. I am also fine with removing the inspect part again. What is the general opinion here: should it better stay or do you think it should be removed as well?

I actually came up with the idea about comparing them after our discussion how to handle Weak(Map/Set) and when looking at the alternatives, this seemed to be the best way to go. Especially as there were people having strong opinions about any of the former ideas.

@joyeecheung @TimothyGu @devsnek what do you personally think is the best way to progress there? I actually wrote the PR instead of writing a summary of the ideas but I guess that is the best to do and then to further see where that leads to?

devsnek · 2018-03-11T01:00:07Z

i'm +1 on inspect, i think its been made pretty clear to users at this point that util.inspect output isn't something to rely on, and its honestly just useful to be able to debug the contents of weakmaps/sets

BridgeAR · 2018-03-11T01:08:36Z

I just removed the third commit about the assert part.

BridgeAR · 2018-03-11T01:09:49Z

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

yosuke-furukawa · 2018-03-13T12:32:12Z

I am also -1, same opinion of @TimothyGu .
WeakSet and WeakMap usecase is to create private object and cache, we should not inspect those content easily.

targos · 2018-03-13T12:48:30Z

What about an option similar to showProxy: Do not introspect those objects by default but allow to do so with a showWeak: true?

yosuke-furukawa · 2018-03-13T14:28:05Z

oops, sorry, I saw this PR and browser implementation.
I don't know assert code is already dropped.
util.inspect can see the WeakMap|Set content but inspect roughly. it is almost similar to Google Chrome DevTools. It is ok for me. no problem.

mcollina

LGTM

BridgeAR · 2018-03-14T19:57:50Z

Ping @TimothyGu

Stale review. PTAL

BridgeAR · 2018-03-21T00:45:18Z

Rebased due to conflicts. I also dismissed the stale review. @TimothyGu please have another look!

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

TimothyGu · 2018-03-22T22:27:22Z

lib/util.js

+  // retrieved ones exist, we can not reliably return the same output).
+  output = output.sort();
+  if (entries.length > maxArrayLength)
+    output.push('... more items');


Nit: the corresponding code in formatArray() is:

node/lib/util.js

Lines 771 to 772 in 982e3bd

if (remaining > 0)

output[i++] = `... ${remaining} more item${remaining > 1 ? 's' : ''}`;

We could do that instead.

I do not see how that is possible. We have to explicitly enter a maximum number of entries we want. This is a upper bound and to be able to show that there are more entries, I actually get one more than requested with the regular maximum. We would have to get all entries each time (by passing through e.g. the maximum number of entries a Array may have) but that would be a significant overhead.

I could get two more items to show "1 more item" and "more items".

Ah I misread it then. LGTM!

TimothyGu · 2018-03-22T22:27:35Z

lib/util.js

@@ -288,6 +295,8 @@ function inspect(value, opts) {
    colors: inspectDefaultOptions.colors,
    customInspect: inspectDefaultOptions.customInspect,
    showProxy: inspectDefaultOptions.showProxy,
+    // TODO(BridgeAR): Deprecate `maxArrayLength` and replace it with
+    // `maxEntriesLength`.


How about just maxEntries?

TimothyGu · 2018-03-22T22:31:41Z

lib/internal/v8.js

+ *
+ * @param {MapIterator} it The iterator that should be cloned.
+ * @returns {MapIterator}
+ */


I don't believe we have used JSDoc annotations anywhere else in the code base. Maybe just use prose instead? (I.e., replace this comment block with just Clone the provided Map Iterator.)

Speaking of documentation, I think a general comment for this entire file would be good. Specifically:

what it is, where these functions are used, maybe a half-sentence intro on V8's natives syntax, and

how it is special compared to rest of the code base (loaded before anything else with a special flag that allows natives syntax).

BridgeAR · 2018-03-22T23:49:46Z

Just as a heads up: I thought about the current implementation again and I actually plan on using @targos suggestion to only show the WeakMap and WeakSet entries in case the showHidden option is activated. I do not think we have to add another option for this as the name fits pretty well. Is that fine for everyone?

1) So far extra keys on an (Set|Map)Iterator were ignored. Those will now be visible. 2) Improve the performance of showing (Set|Map)Iterator by using the cloned iterator instead of copying all entries first. 3) So far the output was strictly limited to up to 100 entries. The limit will now depend on `maxArrayLength` instead (that default is set to 100 as well) and the output indicates that more entries exist than visible.

This adds support for WeakMap and WeakSet entries in `util.inspect`. The output is limited to a maximum entry length of `maxArrayLength`. Fixes: nodejs#19001

BridgeAR · 2018-03-24T11:30:09Z

Comments addressed. I also updated the docs added a note about the inspection being unreliable. PTAL.

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

1) So far extra keys on an (Set|Map)Iterator were ignored. Those will now be visible. 2) Improve the performance of showing (Set|Map)Iterator by using the cloned iterator instead of copying all entries first. 3) So far the output was strictly limited to up to 100 entries. The limit will now depend on `maxArrayLength` instead (that default is set to 100 as well) and the output indicates that more entries exist than visible. PR-URL: nodejs#19259 Reviewed-By: Yosuke Furukawa <yosuke.furukawa@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

This adds support for WeakMap and WeakSet entries in `util.inspect`. The output is limited to a maximum entry length of `maxArrayLength`. PR-URL: nodejs#19259 Fixes: nodejs#19001: Reviewed-By: Yosuke Furukawa <yosuke.furukawa@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

BridgeAR · 2018-03-25T01:22:11Z

Landed in 0fbd4b1, 1029dd3

BridgeAR requested a review from yosuke-furukawa March 9, 2018 16:22

nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. util Issues and PRs related to the built-in util module. labels Mar 9, 2018

TimothyGu previously requested changes Mar 9, 2018

View reviewed changes

BridgeAR force-pushed the inspect-weak-map-set branch from 2e16308 to 4f08813 Compare March 11, 2018 01:08

yosuke-furukawa approved these changes Mar 13, 2018

View reviewed changes

mcollina approved these changes Mar 14, 2018

View reviewed changes

BridgeAR added the semver-major PRs that contain breaking changes and should be released in the next major version. label Mar 14, 2018

BridgeAR requested a review from a team March 14, 2018 19:59

BridgeAR force-pushed the inspect-weak-map-set branch from 4f08813 to 249f427 Compare March 21, 2018 00:42

BridgeAR requested review from jasnell, joyeecheung and TimothyGu March 21, 2018 00:46

TimothyGu approved these changes Mar 22, 2018

View reviewed changes

BridgeAR force-pushed the inspect-weak-map-set branch from 249f427 to 9a4c2f7 Compare March 23, 2018 14:01

BridgeAR changed the title ~~util,assert: improve Weak(Map|Set) support~~ util: improve Weak(Map|Set) support Mar 23, 2018

util: show Weak(Set|Map) entries in inspect

f1e0752

This adds support for WeakMap and WeakSet entries in `util.inspect`. The output is limited to a maximum entry length of `maxArrayLength`. Fixes: nodejs#19001

BridgeAR force-pushed the inspect-weak-map-set branch from 9a4c2f7 to f1e0752 Compare March 24, 2018 11:28

jasnell approved these changes Mar 24, 2018

View reviewed changes

BridgeAR closed this Mar 25, 2018

BridgeAR deleted the inspect-weak-map-set branch April 1, 2019 23:39

lydell mentioned this pull request May 1, 2022

Truncate long Set and Map in util.inspect/console.log/REPL, just like long Array #42933

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

util: improve Weak(Map|Set) support #19259

util: improve Weak(Map|Set) support #19259

BridgeAR commented Mar 9, 2018 •

edited

Loading

BridgeAR commented Mar 9, 2018

TimothyGu left a comment

BridgeAR commented Mar 9, 2018

BridgeAR commented Mar 9, 2018

joyeecheung commented Mar 10, 2018

devsnek commented Mar 10, 2018 •

edited

Loading

BridgeAR commented Mar 11, 2018

devsnek commented Mar 11, 2018 •

edited

Loading

BridgeAR commented Mar 11, 2018

BridgeAR commented Mar 11, 2018

yosuke-furukawa commented Mar 13, 2018

targos commented Mar 13, 2018

yosuke-furukawa commented Mar 13, 2018

mcollina left a comment

BridgeAR commented Mar 14, 2018

BridgeAR commented Mar 21, 2018

TimothyGu Mar 22, 2018

BridgeAR Mar 22, 2018

BridgeAR Mar 22, 2018

TimothyGu Mar 22, 2018

TimothyGu Mar 22, 2018

TimothyGu Mar 22, 2018

BridgeAR commented Mar 22, 2018

BridgeAR commented Mar 24, 2018

BridgeAR commented Mar 25, 2018

	if (remaining > 0)
	output[i++] = `... ${remaining} more item${remaining > 1 ? 's' : ''}`;

util: improve Weak(Map|Set) support #19259

util: improve Weak(Map|Set) support #19259

Conversation

BridgeAR commented Mar 9, 2018 • edited Loading

Checklist

BridgeAR commented Mar 9, 2018

TimothyGu left a comment

Choose a reason for hiding this comment

BridgeAR commented Mar 9, 2018

BridgeAR commented Mar 9, 2018

joyeecheung commented Mar 10, 2018

devsnek commented Mar 10, 2018 • edited Loading

BridgeAR commented Mar 11, 2018

devsnek commented Mar 11, 2018 • edited Loading

BridgeAR commented Mar 11, 2018

BridgeAR commented Mar 11, 2018

yosuke-furukawa commented Mar 13, 2018

targos commented Mar 13, 2018

yosuke-furukawa commented Mar 13, 2018

mcollina left a comment

Choose a reason for hiding this comment

BridgeAR commented Mar 14, 2018

BridgeAR commented Mar 21, 2018

TimothyGu Mar 22, 2018

Choose a reason for hiding this comment

BridgeAR Mar 22, 2018

Choose a reason for hiding this comment

BridgeAR Mar 22, 2018

Choose a reason for hiding this comment

TimothyGu Mar 22, 2018

Choose a reason for hiding this comment

TimothyGu Mar 22, 2018

Choose a reason for hiding this comment

TimothyGu Mar 22, 2018

Choose a reason for hiding this comment

BridgeAR commented Mar 22, 2018

BridgeAR commented Mar 24, 2018

BridgeAR commented Mar 25, 2018

BridgeAR commented Mar 9, 2018 •

edited

Loading

devsnek commented Mar 10, 2018 •

edited

Loading

devsnek commented Mar 11, 2018 •

edited

Loading