FormidableLabs / react-fast-compare

fastest deep equal comparison for React
MIT License
1.59k stars 54 forks source link

Bug/Feature: Support ES6 constructs #36

Closed dickbrouwer closed 4 years ago

dickbrouwer commented 6 years ago

Sets always evaluate to true, I suspect the behavior is similarly wrong for ES6 Map, WeakMap, and WeakSet:

isEqual(new Set([3]), new Set([6]))
=> true
ryan-roemer commented 6 years ago

@chrisbolin -- I've got the start of a branch with initial failing regression tests at https://github.com/FormidableLabs/react-fast-compare/compare/feature/es6-datatypes

I'll be back from parental leave in a week and may be able to div in. I think our work is something like:

chrisbolin commented 6 years ago

Yeah, i'd like to also put a PR into fast-deep-equal for the added support - they currently don't have it: https://github.com/epoberezkin/fast-deep-equal/blob/master/index.js. then if/when they merge we can pull in their code so we haven't strayed too far

ryan-roemer commented 5 years ago

As an update for this, there is now es6 supported upstream (see https://github.com/epoberezkin/fast-deep-equal/blob/master/src/index.jst#L20-L65), but they went a weird route and implemented it by templating the ultimate source file and macro-ing/building out es5 and es6 versions. I think we might be able to just feature detect Map, Set, etc. and keep everything within just one file without too much bloat using upstream as the implementation guide...

chrisbolin commented 5 years ago

thanks for the heads up, @ryan-roemer! I'll check that out

chrisbolin commented 5 years ago

@ryan-roemer what do you think of this plan

This plan has a few benefits...

ryan-roemer commented 5 years ago

I think that generally all sounds good, except I was thinking of making failures more permissive for these constructs... Not sure what React or Victory or Formik (out biggest users) support, but we might want to track them.

And all I'm thinking of for permissive is taking something like:

    if (a instanceof Map) {
      // STUFF
    }

and making it:

    if (typeof Map === "function" && a instanceof Map) {
      // STUFF
    }

for all of Map, WeakMap, Set, WeakSet as applicable (forget what they support upstream).

For:

    if (a.constructor.BYTES_PER_ELEMENT && (
      a instanceof Int8Array ||
      a instanceof Uint8Array ||
      a instanceof Uint8ClampedArray ||
      a instanceof Int16Array ||
      a instanceof Uint16Array ||
      a instanceof Int32Array ||
      a instanceof Uint32Array ||
      a instanceof Float32Array ||
      a instanceof Float64Array ||
      a instanceof BigInt64Array ||
      a instanceof BigUint64Array
    )) {
      // STUFF
    }

I think we can just leave that as-is as the check for BYTES_PER_ELEMENT will avoid parse errors for old browsers...

ryan-roemer commented 4 years ago

Oh, haha, WeakSet and WeakMap are not iterable, which I think means we're out of luck.

Should we now close this one as complete @chrisbolin ?

chrisbolin commented 4 years ago

good call. Weak* support would be (1) very hard and (2) probably pointless. I haven't see Weak* being passed as props, or really used at all.

chrisbolin commented 4 years ago

I stand corrected 😂

https://github.com/jaredpalmer/formik/blob/ae06d2b384d7395bf756d631327d2dfdb0dbd3dc/packages/formik/src/utils.ts#L155

chrisbolin commented 4 years ago

But the fact still stands that it would be very hard (or impossible) to implement, based on the very design of the Weak objects:

there is no list of current objects stored in the collection. WeakSets are not enumerable.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/WeakSet