(!strict || (typeof val1 === 'object' && val1 !== null) || typeof val1 === 'function')?

We don't need to search for functions because the only way deepEqual will consider functions equivalent is through reference equality. The null check makes some sense, although I think it'll make a very small difference in practice because putting null in a set and using null as a key are rare. (In my code, null popping up in either of those places is usually an indication of a bug)

Its a minor point, but do you think its still worth putting in the null check? I'm indifferent either way, but happy to add it if you think the marginal speed improvement is worth the marginal extra complexity of that check.

@josephg Yes, I’d say it’s worth it… there might be cases where the difference is not so marginal, and I think it’s okay for Node core to sacrifice a tiny bit of readability for a small performance gain.

Done

If I am reading the code correctly, is it similar to...

for (const val1 of a) {
  if (!b.has(val1)) {
    if (strict) { return false; }
    if (typeof val1 !== 'object' || val1 === null)) { return false; }

   // the following can be wrapped into a funciton...
    var bHasSimilarElement = false;
    for (const val2 of b) {
      if (_deepEqual(val1, val2, strict, actualVisitedObjects)) {
        bHasSimilarElement = true;
        break;
      }
    }

    if (!bHasSimilarElement) {
      return false;
    }
  }
}

?

Personally I prefer to stay away from labels...

Also on #12142 (comment), I think the primitive check can be replaced with util.isPrimitive(val) so it is more inline with objEquiv and probably more future proof

That transformation is close... Your version is equivalent to if (strict || ...) whereas it should be if (strict && ....)

But yes, I could write the function that way. I'm usually a little happier with early returns for stuff like that, but I know some people don't like them and I'm not sure where nodejs stands on it.

As for ditching the loop labels, I'm not convinced. Your version is twice as long. If I pull that code out into a single-use function then it'd lose some locality. And we'd have to have another similar-but-different copy of that function for mapEquiv as well. (I'd love to write something we can reuse, but it would need to yield back all of the matches for mapEquiv, which means taking a callback, returning an iterator or being a generator function. I'd rather avoid all of those options for this.

As for replacing it with stuff in util, I can either do (!isPrimitive(val1) && !isFunction(val1)) or isObject(val1). The latter has exactly the behaviour we want, but the fact that it returns true for arrays is weird and non-obvious to me. Do you have a preference?

Oh yes, missed the conjunction..so on a higher-level, this is:

for (const val1 of a) {
  if (!b.has(val1) &&
      (strict && util.isPrimitive(val1) || isFunction(val1)) ||
      !hasDeepEqual(b, val1, strict, actualVisitedObjects)) {
    return false;
  }
}

?
EDIT: the first version is..totally wrong.. :S

Also I don't think creating a function that takes callbacks is a good idea either, but that's not what I am proposing here..just think small functions and conjunctions would be easier to read than labels

EDIT: .. doesn't really matter if those small functions can be reused
(mapEquiv can call its own small functions too..). Although function calls incur overhead, but since deepEqual has too many complex logic and we use a pattern like this already, I think it's fine to trade this overhead for readability. As you can see we don't inline setEquiv even though it's not reusable, and we don't make a function taking callbacks to abstract over setEquiv/mapEquiveither. All for readability, eh ;)

Yes thats right - except we can also exclude functions from the search. And I think the logic is still wrong. I'm reasonably sure this is correct:

  for (const val1 of a) {
    if (!b.has(val1)
      && ((strict && (util.isObject(val1) || util.isFunction(val1)))
        || setHasSimilarElement(b, val11, strict, actualVisitedObjects)))
      return false;
    }
  }

(And if its not correct I think the tests should catch any mistakes.)

I just deleted a bunch of text justifying not changing the code, but I think I'm just getting exhausted 7 reviewers rubbing away at, now, stylistic changes. But I agree - the code is cleaner the way you've suggested. I'll update the PR...

Also also..to be clear, I don't actually feel strong enough about the labels to block this PR, because support for Maps and Sets are definitely appreciated and outweight readability preferences! Although on the other hand, I do feel stronger about the doc update ...even the comments in_deepEqual are updated, surely the documentation deserves some love right ;)
EDIT: oh I saw the doc update now, thanks

See my comments below - I updated the documentation a couple hours ago.

Also I was still wrong - here's the actually correct logic. At least, the tests pass with this version. I'm pretty sure its right (though I convinced myself the last version was right too).

    if (!b.has(val1)
        && (!strict || (!util.isObject(val1) && !util.isFunction(val1)))
        && !setHasSimilarElement(b, val1, strict, actualVisitedObjects)) {
      return false;
    }