Avoid shouldInstrument ReferenceError on ES6 types

[bz2]

To check for builtin types, the shouldInstrument file creates a map with various constructors as the keys. However, as WeakSet is not used, there's no guarantee it's been polyfilled in for browsers that lack support like IE11, which leads to a runtime ReferenceError when proxyequal is imported.

Instead key on the constructor name, which gracefully handles cases where the browser does not have one of the types that need special handling.

Example load failure on IE11 with 2.0.6 currently:

"ReferenceError: 'WeakSet' is undefined
   at Anonymous function (eval code:26:1)
   at eval code (eval code:1:29)
   at ./node_modules/proxyequal/lib/shouldInstrument.js (...:355:1)
   at __webpack_require__ (...:767:12)

[theKashey]

No, you fixed a wrong mistake in a wrong way.

• original problem: "ReferenceError: 'WeakSet' is undefined
• the line which may cause the problem - [WeakMap, collectionHandlers]. - WeakMap is not defined
• does the same access to the undefined WeakMap exist in the patch? Yes. And the problem still exists.

How to fix? Easy - [window.WeakMap, collectionHandlers], we really need that global, defined on the window, and it's safe to access it, even if it is not defined.

Result code:

const handlers = new Map([
   [window.Map, collectionHandlers]
    ....
].filter(([x]) => !!x) // filter out all non-resolved globals
);

[bz2]

@theKashey Please reconsider:

• In Javascript, property keys are always strings, and are not evaluated.
• Keying on constructors is not needed, and is actually harmful.
• The existing tests pass with this change.

Why is keying on constructors bad?

So, on import the current code takes the object with name eg. 'Map' from the global object. In shouldInstrument() it verifies that the passed value is a function that is a property of the global object with the same name. Because of this, it's unimportant to store the constructor in handlers as we've verified that constructor is the current globally bound object of that name.

So, why bad? What if Map is pollyfilled? Then whether this code functions correctly depends on if the pollyfill import happens before or after the shouldInstrument import, whereas when binding by name there is no dependency on import order.

[theKashey]

But in the same time it's unsafe. It's not doing what it should, and not checking what it have to.

[bz2]

@theKashey Please define 'safe' in this context. Either implementation permits changes the the global object provided the naming of the replacement objects matches, the period in which that is permitted differs. It is already possible to break the implementation by changing global objects or their prototypes. What safety is saving the constructor providing?

[theKashey]

Sorry, but you are absolutely right 😁

[bz2]

@theKashey Thank you very much.

[theKashey]

I'll release it later today. Thank you for your contribution.