frptools / collectable

High-performance immutable data structures for modern JavaScript and TypeScript applications. Functional interfaces, deep/composite operations API, mixed mutability API, TypeScript definitions, ES2015 module exports.
MIT License
273 stars 14 forks source link

Question: is the built-in hash function sufficient? #67

Open njlr opened 6 years ago

njlr commented 6 years ago

I was looking through the source and found that the hash function is quite simple:

import { stringHash } from '../common';

export function findHash (key: any): number {
  const hash: number = typeof key === 'number'
    ? key
    : typeof key === 'string'
      ? stringHash(key)
      : Math.abs(stringHash(JSON.stringify(key)));

  return hash;
}

I can see two problems that might occur with this.

First, JSON.stringify can be sensitive to property order, which might lead to unexpected behaviour:

> JSON.stringify({ x: 1, y: 2 })
'{"x":1,"y":2}'
> JSON.stringify({ y: 2, x: 1 })
'{"y":2,"x":1}'

Second, (and please correct me if I missed something!) it does not leave a possibility for customization. I think the set and map modules should be parameterized on a tuple of hash and equals. A default can be provided for convenience.

axefrog commented 6 years ago

Hey, really sorry about the slow reply. I foolishly read the issue on my phone before going to sleep, and then forgot to follow up.

The HashMap collection was actually ported from another package (see the contributor credits), and the intent was to upgrade the hashing function there to use what's in the core library. It hasn't come up as an issue for me yet though, which is why it's remained on the backburner. I'm open to pull requests on this one. Note that what's in the core library should be ok in theory, but again if you see any issues there, I'm very much open to contributions that help me improve things. This project is very much alive, despite the apparent long periods of inactivity - I just have several projects I'm working on in tandem, and being just one person, I only have so much productive bandwidth to allocate at any given time.