mattbierner / hamt

Javascript Hash Array Mapped Trie
MIT License
251 stars 16 forks source link

Allow to provide defaultValue to hamt.modify and hamt.modifyHash #14

Closed dashed closed 8 years ago

dashed commented 8 years ago

Provide any given defaultValue, via either hamt.modify or hamt.modifyHash, to function f if entry by key, key, doesn't exist.

Note: This changes the semantics of calling the transform function with no arguments when an entry doesn't exist with a given key.

This aligns with notSetValue semantics of Immutable.js. e.g. http://facebook.github.io/immutable-js/docs/#/Map


I have a branch with this change: https://github.com/dashed/hamt/tree/defaultValue

If interested in this change, I can flesh out with more tests and create a PR.

mattbierner commented 8 years ago

This sounds interesting but do I have a few thoughts on it.

modify is currently called with zero arguments in the case where m does not exist. For example:

var h = hamt.empty;

var f = function(x) {
    return arguments.length === 0 ? 0 : x + 1;
};

h = h.modify('key', f)  // { key: 0 }
h = h.modify('key', f)  // { key: 1 }

This is not perfect, not least because the safest approach outlined here does not work with lambda functions, but this is useful behavior for some cases because a single function handles everything (this is what get uses to do inserts).

I'm not entirely opposed to adding a defaultValue argument, however, the new behavior should only apply when a defaultValue argument is present. The argument order would probably be better expressed as:

export const modify = (f, key, defaultValue, map) => {
    if (arguments.length >= 4) {
       // new, defaultValue behavior
    } else {
       // current behavior
    }
};

As for the implementation, I believe it may be cleaner to handle defaultValue within the modify API and not spread the concept of defaultValues into the internal _modify implementations.

Let me know what you think.

dashed commented 8 years ago

... however, the new behavior should only apply when a defaultValue argument is present.

I think we can compromise on this; I created a preliminary unfinished PR to demonstrate this https://github.com/mattbierner/hamt/pull/19