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

[mergeWithDraft] | ES6 Set #1

Open
bradennapier opened this issue Jul 25, 2018 · 0 comments
Open

[mergeWithDraft] | ES6 Set #1

bradennapier opened this issue Jul 25, 2018 · 0 comments
Labels
help wanted Extra attention is needed

Comments

@bradennapier
Copy link
Member

bradennapier commented Jul 25, 2018

Summary

Conducting deep merges with Set values is inherently difficult to do for multiple reasons. This difficulty is compounded with the immutable data structure that immuta builds using ES6 Proxies.

tl;dr - Set works, but it is likely to cause unwanted effects. Use Array instead where possible or modify the set using recursion manually and never assume Set given from immuta is going to preserve the order they were added in.

Possible Solution?

Possible solution first since most of us have some form of ADD and prob can't handle all the words below without understanding what the end looks like ;-) (or maybe that's just me!).

The only possibility I can imagine of making Set work exactly as expected would be to rebuild Set using an array when we receive one, manage the lifecycle throughout the mutations that are given, and return a Set which maintains the insertion order.

This would end up with essentially a double-proxy upon Set's and would require maintaining a few extra levels of state within our state descriptors.

The Problem

The root of our problem is that Set doesn't have (and has no reason to have) any mechanism to "get" values. We also can't rely on the order of values due to the fact Set guarantees that all values in the Set are unique.

Take this case:

const k1 = { foo: 1 };
const k2 = { bar: 2 };
const k3 = { baz: 3 };

const set = new Set([k1, k2, k3]);

// now lets consider a situation where 
// we do something like:

set.add(k1);

In this example, the set is unchanged in the last .add() as k1 already exists within the set. So while we might assume k1 is where we expect it to be, there are many situations where it isn't if we aren't extremely careful with them.

In addition to this, our only real mechanism for capturing the Proxy objects inside of this set are to iterate the set manually. We do not generate any proxies or modify anything until you read so until you iterate the set, its just a native Set { k1, k2, k3 }.

Say you did something like:

const k1 = { foo: 1 };
const k2 = { bar: 2 };
const k3 = { baz: 3 };

const state = {
  set: new Set([k1, k2, k3]),
};

const next = immuta(state, draft => {
  draft.set.forEach(v => {
    if (v.foo || v.bar) {
      v.qux = 4;
    }
  });
});

printDifference(state, next);

You may be surprised in this case what the result is. Since we need to modify the object reference deeply, the only possibility is to remove the base value and add our shallow copied copy to the set, changing the ordering in this case:

image

Or if we print the Set's of state and next directly:

Set { { foo: 1 }, { bar: 2 }, { baz: 3 } }; // state
Set { { baz: 3 }, { foo: 1, qux: 4 }, { bar: 2, qux: 4 } }; // next

Because of this, simply iterating the new set and merging with values in the same index of the source set (which is how we originally planned to do this) is problematic.

Lets make matters worse

We actually have further issues here. Since even checking if a value exists on Set means you must already have a reference to that value (if its an object), we actually can't really ever properly proxy against a Set's values unless we iterate it (using something like forEach or for...of. We handle this fine in immuta's standard functions (as shown above) where we iterate and optionally mutate, but merge either would need to keep pretty extensive history throughout the lifecycle, or it would need to simply be transformed into an array in order to be any kind of efficient here.

All-in-all - Set is simply not generally a good idea and I am not sure I see any solution that can be made easy to use.

This is why, in the end, merging sets in general is problematic.

How it Works Now

For now, mergeWithDraft will simply add any values that did not previous exist into the draft. This can definitely cause unwanted issues, and it may need to be changed to either ignore or replace the Set all together (with a warning).

Take this example where we may want to mutate the k1 directly, in this case we might try something like that would work with arrays like this:

const k1 = { foo: 1 };
const k2 = { bar: 2 };
const k3 = { baz: 3 };

const state = {
  set: new Set([k1, k2, k3]),
};

const next = immuta(state, draft => {
  const qux = { qux: 4 };
  mergeWithDraft(draft, {
    set: new Set([qux, {}, qux]),
  });
});

printDifference(state, next);

Which would ultimately end with:

Set { { foo: 1 }, { bar: 2 }, { baz: 3 } }
Set { { foo: 1 }, { bar: 2 }, { baz: 3 }, { qux: 4 }, {} }

This is due to:

  1. Set keys must be unique (only adds qux once when calling new Set() here.
  2. We can not guarantee that the order is what you expect it to be, so we can't assume that iterating the current set will produce the object you want to merge with.
  3. We end up checking if qux is on set, and add it to the Set if it doesn't.

If we compare to the same with array:

const k1 = { foo: 1 };
const k2 = { bar: 2 };
const k3 = { baz: 3 };

const state = {
  arr: [k1, k2, k3],
};

const next = immuta(state, draft => {
  const qux = { qux: 4 };
  mergeWithDraft(draft, {
    arr: [qux, {}, qux],
  });
});

console.log(state.arr);
console.log(next.arr);

We likely end with the exact results you are expecting:

[ { foo: 1 }, { bar: 2 }, { baz: 3 } ]
[ { foo: 1, qux: 4 }, { bar: 2 }, { baz: 3, qux: 4 } ]

What about ES6 Map?

ES6 Map is a little bit different. immuta treats the keys of Map as they are. We do not proxy them and we do not attempt to provide specific drafts for them. They are not immutable and any attempt to do this introduces an insane amount of possible issues and side effects (trust me, we tried ;-))

Since we have .get() the merge works exactly how we would expect and can be done fairly efficiently. Note that we do need to proxy the function calls for map - but this can work out to an advantage in some cases.

const k1 = { foo: 1 };
const k2 = { bar: 2 };
const k3 = { baz: 3 };
const qux = { qux: 4 };

const state = {
  map: new Map([[k1, k1], [k2, k2], [k3, k3]]),
};

const next = immuta(state, draft => {
  mergeWithDraft(draft, {
    map: new Map([[k1, qux], [k3, qux]]),
  });
});

console.log(state.map);
console.log(next.map);
Map {
  { foo: 1 } => { foo: 1 },
  { bar: 2 } => { bar: 2 },
  { baz: 3 } => { baz: 3 } }
Map {
  { foo: 1 } => { foo: 1, qux: 4 },
  { bar: 2 } => { bar: 2 },
  { baz: 3 } => { baz: 3, qux: 4 } }

With the printDifference looking like:

image

Also note that in this example, we would probably benefit from using .at version of mergeWithDraft

const k1 = { foo: 1 };
const k2 = { bar: 2 };
const k3 = { baz: 3 };
const qux = { qux: 4 };

const state = {
  map: new Map([[k1, k1], [k2, k2], [k3, k3]]),
};

const next = immuta(state, draft => {
  mergeWithDraft.at(draft, ['map', k1], qux);
  mergeWithDraft.at(draft, ['map', k3], qux);
});

// OR

const next = immuta(state, draft => {
  // reduce draft/proxy calls whenever possible
  const mapDraft = draft.map;
  mergeWithDraft.at(mapDraft, [k1], qux);
  mergeWithDraft.at(mapDraft, [k3], qux);
});

One interesting property of how we handle things is that WeakMap is actually possible as long as you create the WeakMap originally then switch to using maps!

const k1 = { foo: 1 };
const k2 = { bar: 2 };
const k3 = { baz: 3 };

const state = {
  map: new WeakMap([[k1, k2]]),
};

const next = immuta(state, draft => {
  mergeWithDraft(draft, {
    map: new Map([[k1, k3]]),
  });
});
console.log(state.map instanceof WeakMap, state.map.get(k1));
console.log(next.map instanceof WeakMap, next.map.get(k1));
true { bar: 2 }
true { bar: 2, baz: 3 }

Anyway, this has gotten quite long.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

1 participant