Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug/Feature: Support ES6 constructs #36

Closed
dickbrouwer opened this issue Sep 22, 2018 · 10 comments
Closed

Bug/Feature: Support ES6 constructs #36

dickbrouwer opened this issue Sep 22, 2018 · 10 comments
Labels
enhancement New feature or request verified Verified issue

Comments

@dickbrouwer
Copy link

dickbrouwer commented Sep 22, 2018

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
Copy link
Member

ryan-roemer commented Sep 22, 2018

@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:

  • Set support
  • Map support
  • Typed arrays support
  • Regression tests for all
  • Make sure the library correctly detects / permissively handles browsers / environments without those constructs.
  • WeakMap support
  • WeakSet support

@ryan-roemer ryan-roemer added enhancement New feature or request verified Verified issue labels Sep 22, 2018
@ryan-roemer ryan-roemer changed the title Broken for Sets Bug/Feature: Support ES6 Set, Map constructs Sep 22, 2018
@chrisbolin
Copy link
Contributor

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
Copy link
Member

ryan-roemer commented Jul 11, 2019

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
Copy link
Contributor

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

@chrisbolin
Copy link
Contributor

@ryan-roemer what do you think of this plan

  • make a v3 beta for react-fast-compare
  • make our fork from the built es6 file: https://runpkg.com/?fast-deep-equal@3.0.0-beta.1/es6/index.js
  • if our users like v3beta and fast-deep-equal releases their v3beta, we release a version 3 that only supports es6. if users cannot use es6 or polyfill, they have react-fast-compare v2 to use

This plan has a few benefits...

  • no complex JS templating or build process
  • direct fork of fast-deep-equal, so we aren't adding and maintaining too much
  • support for modern systems only. WeakMap, etc is part of ES6 and has support in 95% of global browsers and in node all the way back to 6.5.0 (August 2016)

@ryan-roemer
Copy link
Member

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...

chrisbolin pushed a commit that referenced this issue Jan 29, 2020
- Update library to include ES.next support for `Map`, `Set`, `ArrayBuffer`. Part of #36
- Update to `fast-deep-equal@3.1.1` with modified support for ES.next data types.
- Upgrade lots of `devDependenices`
- Use `fast-deep-equal` tests directly in our correctness tests.
- Update CI to modern Node.js versions.
- **Note**: There's a bug / limitation of `Set` comparisons whereby objects are compared by reference not value. Tracked at #50 . In our `yarn benchmark`, `lodash.isEqual` gets test differences because it correctly handles those.
@ryan-roemer ryan-roemer changed the title Bug/Feature: Support ES6 Set, Map constructs Bug/Feature: Support ES6 (WeakSet, WeakMap) constructs Jan 31, 2020
@ryan-roemer ryan-roemer changed the title Bug/Feature: Support ES6 (WeakSet, WeakMap) constructs Bug/Feature: Support ES6 constructs Jan 31, 2020
@ryan-roemer
Copy link
Member

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
Copy link
Contributor

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
Copy link
Contributor

@chrisbolin
Copy link
Contributor

chrisbolin commented Jan 31, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request verified Verified issue
Projects
None yet
Development

No branches or pull requests

3 participants