refactor(path-compare): Improve documentation and reduce implementation code size

[gibson042]

Refs: #2787

Description

Applies suggestions from #2787 (review) .

[kriskowal]

I do not feel strongly about this: I agree that the reduce is not ideal, but I prefer a solution that improves both legibility and is lest wasteful with allocations. To that end, I’d prefer to keep this as-is or go the other direction: replace cumulativeLength’s internals with a C-style for loop and call that once on each of these lines.

[gibson042]

I understand the concern, but in practice I just can't see those allocations at all... I think the internal allocation implied by strings.join('').length doesn't actually happen in V8, as observed by comparing output from the following:

$ esbench.mjs -h V8 \
  -s 'const arr = Array.from({ length: 1000 }, (_, i) => `element${i}`);' \
  join:'ret = []; for(let i = 0; i < arr.length; i++) ret.push( arr.slice(i).join("") );' \
  --dump \
  | node --heap-prof-dir="$(pwd)" --heap-prof --heap-prof-interval 100
join sync 0.04 ops/ms (24.5e+6 ns/op) after 123 1-count observations

$ esbench.mjs -h V8 \
  -s 'const arr = Array.from({ length: 1000 }, (_, i) => `element${i}`);' \
  join:'ret = []; for(let i = 0; i < arr.length; i++) ret.push( arr.slice(i).join("").length );' \
  --dump \
  | node --heap-prof-dir="$(pwd)" --heap-prof --heap-prof-interval 100
join sync 0.04 ops/ms (24.5e+6 ns/op) after 123 1-count observations

The first captures output from join and so requires string allocations (takeObservation claims 5 MB, dominated by join), while the second does not (takeObservation claims just 18 KB, a third of which is Array.from).

[boneskull]

Is it really worth optimizing? Given where pathCompare is consumed, the bottleneck is I/O.

[boneskull]

No blockers

[boneskull]

This rule exists because:

Nesting ternary expressions can make code more difficult to understand.

It's hard for me to disagree with that.

If we're going to disable it on a one-off basis (and again on L21 above), we should have a justification for doing so. Otherwise, it may be better to leave it for now, then propose we disable the rule because we feel it isn't buying us anything.

Ideally, I'd like a rule that demands an extra comment for each one-off disabled rule (much like ban-ts-comment configured with 'ts-expect-error': 'allow-with-description'). 😄 I bet someone solved this already...

[gibson042]

What I think we really want is a maximum ternary nesting depth of 1 rather than 0, to allow for precisely this kind of use in comparison functions ($equal ? 0 : $aLessThanB ? -1 : 1).

[boneskull]

Is it really worth optimizing? Given where pathCompare is consumed, the bottleneck is I/O.