[DevTools] Replace constructor.name checks with constructor checks

[Willie-Boy]

Summary

There is a question in the code of utils.js asking for a better way to check for an ArrayBuffer:

HACK This ArrayBuffer check is gross; is there a better way?

Replacing the current check (data.constructor && data.constructor.name === 'ArrayBuffer') with data.constructor && data.constructor === ArrayBuffer is arguably a better and more efficient way.

How did you test this change?

In addition to running the existing tests, I compared the speeds of the checks:

let data = new ArrayBuffer(1000000);

// Measure the performance of using constructor
console.time('constructor check');
for (let i = 0; i < 1000000; i++) {
  if (data.constructor && data.constructor === ArrayBuffer) {
    // Do something
  }
}
console.timeEnd('constructor check');

// Measure the performance of using constructor.name
console.time('constructor.name check');
for (let i = 0; i < 1000000; i++) {
  if (data.constructor && data.constructor.name === 'ArrayBuffer') {
    // Do something
  }
}
console.timeEnd('constructor.name check');

This resulted in significant speed differences:

constructor check: 4.143798828125 ms
constructor.name check: 29.721923828125 ms

[Willie-Boy]

@mondaychen, this PR is probably micro-optimization, but at least it provides some confidence in the checks. These checks could have been possible, too, but they didn't pass the existing tests:

data instanceof ArrayBuffer
Object.prototype.toString.call(data) === '[object ArrayBuffer']

[mondaychen]

This seems fine to me.
@bvaughn do you have any thoughts?

[bvaughn]

This code was from 2019 (#17579); I don't remember exactly why it was written that way.

This is what we were doing before that didn't work in some cases:

} else if (data instanceof ArrayBuffer) {
  return 'array_buffer';

I think checking the constructor name was to guard against cross-realm/iframe issues where there are different ArrayBuffer globals.

Looking at how this library does it, for example, seems to support that the check isn't so simple:
https://github.com/inspect-js/is-array-buffer/blob/main/index.js

I would vote not to change this without good reason. What's the actual motivation here?

[Willie-Boy]

Thank you, @bvaughn, for reviewing this!

Interestingly, using the is-array-buffer module that you mentioned throws the same error as using instanceof:

FAIL  packages/react-devtools-shared/src/__tests__/inspectedElement-test.js (43.093 s)

...

 ● InspectedElement › should support Proxies that dont return an iterator

   expect(received).toMatchInlineSnapshot(snapshot)

   Snapshot name: `InspectedElement should support Proxies that dont return an iterator 1`

   - Snapshot  - 8
   + Received  + 4

     {
       "proxy": {
         "$$typeof": Dehydrated {
   -       "preview_short": ƒ () {},
   -       "preview_long": ƒ () {},
   +       "preview_short": ƒ () {},
   +       "preview_long": ƒ () {},
         },
         "Symbol(Symbol.iterator)": Dehydrated {
   -       "preview_short": ƒ () {},
   -       "preview_long": ƒ () {},
   -     },
   -     "constructor": Dehydrated {
   -       "preview_short": ƒ () {},
   -       "preview_long": ƒ () {},
   +       "preview_short": ƒ () {},
   +       "preview_long": ƒ () {},
         },
       },
     }

It seems that the is-array-buffer module works similarly as instanceof:

const isArrayBuffer = require('is-array-buffer');

class MyArrayBuffer extends ArrayBuffer {}

const myArrayBuffer = new MyArrayBuffer(1);
const newArrayBuffer = new ArrayBuffer(1);

console.log(isArrayBuffer(myArrayBuffer)); // true
console.log(myArrayBuffer instanceof ArrayBuffer); // true
console.log(isArrayBuffer(newArrayBuffer)); // true
console.log(newArrayBuffer instanceof ArrayBuffer); // true

Whereas both data.constructor && data.constructor.name === 'ArrayBuffer' and data.constructor && data.constructor === ArrayBuffer return:

console.log(myArrayBuffer.constructor.name === 'ArrayBuffer'); // false
console.log(myArrayBuffer.constructor === ArrayBuffer); // false
console.log(newArrayBuffer.constructor && newArrayBuffer.constructor.name === 'ArrayBuffer'); // true
console.log(newArrayBuffer.constructor && newArrayBuffer.constructor === ArrayBuffer); // true

I would vote not to change this without good reason. What's the actual motivation here?

Your old question about a better way motivated me to search for one. It seems that the solution that you came up with (the constructor.name check) is actually really good. The one I suggested is just a bit cleaner and less expensive. Functionally they are equivalent. To be honest, I'm not entirely sure if my suggestion is worth implementing since the difference between the approaches is rather small. At the end of the day, the decision is yours! 🙂

[bvaughn]

I appreciate your following up with the extra info!

I would vote not to change this without good reason. What's the actual motivation here?

Your old question about a better way motivated me to search for one. It seems that the solution that you came up with (the constructor.name check) is actually really good. The one I suggested is just a bit cleaner and less expensive. Functionally they are equivalent. To be honest, I'm not entirely sure if my suggestion is worth implementing since the difference between the approaches is rather small. At the end of the day, the decision is yours! 🙂

My concern is still about cross-realm/iframe issues. I think I remember seeing something about that, which is maybe why I used constructor.name instead of explicitly comparing the global type. (The former would be a bit more forgiving.)

[Willie-Boy]

My concern is still about cross-realm/iframe issues. I think I remember seeing something about that, which is maybe why I used constructor.name instead of explicitly comparing the global type. (The former would be a bit more forgiving.)

It seems that you really thought this through back then, because:

Welcome to Node.js v19.0.0.
Type ".help" for more information.
> (new ArrayBuffer(1)).constructor === ArrayBuffer
true
> (vm.runInNewContext('new ArrayBuffer(1)')).constructor === ArrayBuffer
false
> (new ArrayBuffer(1)).constructor.name === 'ArrayBuffer'
true
> (vm.runInNewContext('new ArrayBuffer(1)')).constructor.name === 'ArrayBuffer'
true

Well, this was an interesting exercise. Thank you so much for your time!

[bvaughn]

No problem! Thanks for the speedy responses.