util: add support for maxArrayLength when inspecting Set and Map

[cola119]

related #42933

This PR added maxArrayLength option to Set and Map.
Currently the string print of Set and Map with many entries is a little unreadable since all entries are printed at once. On the contrary, this is not the case at a large array since the print will cut off thanks to maxArrayLength option.

It is inconsistent that maxArrayLength supports WeakMap and WeakSet but not Map and Set and I think supporting them will be helpful for users.

[targos]

@nodejs/util

[BridgeAR]

The implementation seems fine. The option's name is just not ideal and adding another option would also be a bit much. We might be able to change the name by adding maxItemLength and use a getter and setter for maxArrayLength that should sets the new variable instead. What do others think?

[cjihrig]

We might be able to change the name by adding maxItemLength and use a getter and setter for maxArrayLength that should sets the new variable instead.

FWIW, Deno uses iterableLimit for this instead of a separate option for each iterable type.

[cola119]

util.inspect has maxStringLength and maxArrayLength option to specify the length of items and the default value is 100 and 10000 respectively. Would unifying them into something like iterableLimit be a breaking change of the default value?
Personally replacing (by preparing getter and setter)maxArrayLength with maxItemLength and deprecating maxArrayLength sounds good to me.

[cola119]

@BridgeAR I implemented a new maxItemLength option to support Map and Set as an alternative tomaxArrayLength as a trial. 34c3dd4
It seems more consistent in terms of naming and functionality. What do you think?

[ljharb]

it is very weird that the default options have a getter/setter - defaults are commonly spreaded with options to create a new object, so that the getter/setter will be lost.

I think it would be cleaner for behavior to be defined with only data properties, so that it’s possible for users to understand what’s going on.

[ljharb]

these can be done inline, no?

[cola119]

the getter/setter will be lost

That's right... But If I define it inline, I don't know how to assign a default value for maxArrayLength when defaultOptions.maxArrayLength is changed before inspect.

const array = [1,2,3];
util.inspect.defaultOptions.maxArrayLength = 1;
util.inspect(array); // now [ 1, 2, 3 ] but should be [ 1, ... 2 more items ]

Since maxArrayLength is essentially identical to maxItemLength, defaultOptions.maxArrayLength is no longer available? Could you give me some advice on this?

node/lib/internal/util/inspect.js

Lines 304 to 334 in 34c3dd4

	function inspect(value, opts) {
	// Default options
	const ctx = {
	budget: {},
	indentationLvl: 0,
	seen: [],
	currentDepth: 0,
	stylize: stylizeNoColor,
	showHidden: inspectDefaultOptions.showHidden,
	depth: inspectDefaultOptions.depth,
	colors: inspectDefaultOptions.colors,
	customInspect: inspectDefaultOptions.customInspect,
	showProxy: inspectDefaultOptions.showProxy,
	maxItemLength: inspectDefaultOptions.maxItemLength,
	maxStringLength: inspectDefaultOptions.maxStringLength,
	breakLength: inspectDefaultOptions.breakLength,
	compact: inspectDefaultOptions.compact,
	sorted: inspectDefaultOptions.sorted,
	getters: inspectDefaultOptions.getters,
	numericSeparator: inspectDefaultOptions.numericSeparator,
	};
	ObjectDefineProperty(ctx, 'maxArrayLength', {
	__proto__: null,
	get() {
	return ctx.maxItemLength;
	},
	set(val) {
	ctx.maxItemLength = val;
	},
	enumerable: true,
	});

[ljharb]

I think the logic needs to always check both fields, instead of trying to "fix" it in the defaults. That way it also handles when users pass both options.

[targos]

What about this logic:

• No default value
• If options.maxItemLength is not undefined, use it
• Otherwise, use options.maxArrayLength

[ljharb]

Seems much simpler to me.

[aduh95]

I mean, if you or someone else were to open an alternative PR with the design you prefer, that could be discussed, but as is, this PR is ready, and does improve the current API; we can't force anyone to work on it, it wouldn't be fair to block this PR if no one is going to work on the alternative API design anyway.

[ljharb]

I would be happy to work on it; the blocker is fear of potential objections by collabs, and fear of biasing existing PRs.

Can this PR wait while i work on the changes? I can PR them into @cola119’s fork so this PR can be updated.

[aduh95]

the blocker is fear of potential objections by collabs

Hopefully the fear of a not-as-ideal design landing is greater than the fear of being blocked – and FWIW, I don't see a single reason for your PR to be blocked.

Can this PR wait while i work on the changes?

If you get an open (or draft) PR before we got a green CI on this PR (which given the current state of the CI is very achievable), that's a given. Otherwise, we can land this PR and add backport-blocked-v18.x PRs that should land on the v18.x-staging branch but are blocked by another PR's pending backport. label to give you more time.

[ljharb]

Either would be appreciated. I'll work on it over the next few days.

[cola119]

I welcome to wait your implementation. Thank you.

[BridgeAR]

LGTM. Let's check CITGM in this case as some libraries might depend on a specific output in this case. I do not consider it a breaking change, in case we do not find anything on CITGM.

[nodejs-github-bot]

Landed in 71ca6d7

[ljharb]

@F3n67u i thought we were waiting until i had a chance to update the PR? #43576 (comment)

[aduh95]

@ljharb it won't land on Node.js 18.x until your PR is ready as long as it has the label, there's still time to do breaking changes if necessary.

[ljharb]

I appreciate that, but it's quite different to have a ticking clock on it - which wasn't the case prior to this PR being merged.

[F3n67u]

@F3n67u i thought we were waiting until i had a chance to update the PR? #43576 (comment)

@ljharb I am sorry. I missed the comment. I see it is author-ready, ci all green, and approval is enough, so I add it to the commit queue.

I see v18 has just released a new version yesterday. so there is enough time to land your next pr, right?