Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@tklever does this change look like what we discussed?

Want to submit your unrelated test additions as a separate PR? It seems symbols as keys are supported but there's not much test coverage for that and your additions would be a positive

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@leebyron apologies, i linked the performance output on a collapsed thread!

Any progress on a performance analysis of this change?

Performance on master: before.txt

Performance on my branch: after.txt

It does look like there is a negative performance impact to me. What do you think?

I'm curious if this is worth it, when Symbol support as keys already exists, but just aren't extracted from Objects in the constructor, though they are supported via other initialization patterns

The use-case came up for me when trying to do a mergeDeep between multiple object literals which use Symbols as keys. I think Symbol-handling in the constructor is a requirement in that scenario, as instantiation and data-loading of the resulting Map is an implementation detail. Do you know of another way?

I should probably add a mergeDeep Symbol test!

@leebyron bad news! I created a test to cover the scenario which lead me to this request, and this request does not solve for it. I'm reluctant to add the failing test to this request since i believe this request stands on its own (barring the outstanding performance question). I'll dig into the source some more before deciding what to do with the request(s).

here's a preview of the test (which i copied and modified from merge.js) and its output:

  it('deep merges two maps with Symbol keys', () => {
    const a = Symbol('a');
    const b = Symbol('b');
    const c = Symbol('c');
    const d = Symbol('d');
    const e = Symbol('e');
    const f = Symbol('f');
    const g = Symbol('g');
    const m1 = Map({[a]: {[b]: {[c]: 1, [d]: 2}}});
    const m2 = Map({[a]: {[b]: {[c]: 10, [e]: 20}, [f]: 30}, [g]: 40});
    expect(m1.mergeDeep(m2)).toEqual({[a]: {[b]: {[c]: 10, [d]: 2, [e]: 20}, [f]: 30}, [g]: 40});
  });

 FAIL  __tests__/Map.ts
  ● Map › deep merges two maps with Symbol keys

    expect(received).toEqual(expected)
    
    Expected value to equal:
      {Symbol(a): {Symbol(b): {Symbol(c): 10, Symbol(d): 2, Symbol(e): 20}, Symbol(f): 30}, Symbol(g): 40}
    Received:
      Immutable.Map {Symbol(a): {Symbol(b): {Symbol(c): 10, Symbol(e): 20}, Symbol(f): 30}, Symbol(g): 40}
    
    Difference:
    
    - Expected
    + Received
    
    @@ -1,10 +1,9 @@
    - Object {
    + Immutable.Map {
        Symbol(a): Object {
          Symbol(b): Object {
            Symbol(c): 10,
    -       Symbol(d): 2,
            Symbol(e): 20,
          },
          Symbol(f): 30,
        },
        Symbol(g): 40,
      
      at Object.it (__tests__/Map.ts:370:34)
          at Promise (<anonymous>)
          at <anonymous>
      at process._tickCallback (internal/process/next_tick.js:188:7)

Got it! Running the feature and master perf tests again. Will post results soon along with another src+test commit.

@leebyron Good news! I was able to fix my broken test and added a few more to cover additional merge cases with plain objects and keys, as well as Symbol keys and deep merges.

I re-ran the regression tests, on master as a baseline as well as twice on my feature branch for good measure. I'd love your feedback on how the results look. I think I'm looking at some kind of acceptable standard deviation, but I'm not sure what our pass-fail thresholds are.

Master regression tests: master_perf.txt

Feature regression tests (1): feature_perf.txt

Feature regression tests (2): feature_perf2.txt

If you're interested, I can put together a performance test for nested objects with Symbol keys to guard against future regressions on this feature.

Thanks for digging into this further. The performance numbers look a bit concerning to me. I'm seeing a ~25% slow down in creation of Maps, even though our perf tests don't hit this Symbol path.

I think perhaps we should be addressing this in terms of making this use case possible rather than syntactically ideal, in favor of preserving the performance for the common path. Would you agree?

First, I'm not sure we want to necessarily support this for plain Object values, since that can make writing Flow or TypeScript definitions much more challenging - these tools don't yet have great support for plain Objects behaving as maps with Symbols for keys. Perhaps instead we should support this case specifically and only for Maps (as you originally propose).

In that case, we actually do have support for this today, it's just that the constructor would have to use the Iterable<[K, V]> form instead of {[K]: V} form. I think that might be acceptable since we already need to use the Iterable<[K, V]> for any other kind of key that isn't a string (this is the same constructor form that the native ES6 Map uses). I think we should probably preserve the custom {[K]: V} form for Maps with string keys only.

Here's a syntactically different version of your test case which shows that this works today:

const a = Symbol('a');
const b = Symbol('b');
const c = Symbol('c');
const d = Symbol('d');
const e = Symbol('e');
const f = Symbol('f');
const g = Symbol('g');

// Note the use of nested Map constructors, Map() does not do a deep conversion!
const m1 = Map([[a, Map([[b, Map([[c, 1], [d, 2]])]])]]);
const m2 = Map([[a, Map([[b, Map([[c, 10], [e, 20], [f, 30], [g, 40]])]])]]);
const merged = m1.mergeDeep(m2);

// Alternatively, mergeDeep can be directly given a nested set of `Iterable<[K, V]>`
const merged = m1.mergeDeep([[a, [[b, [[c, 10], [e, 20], [f, 30], [g, 40]]]]]]);

expect(merged).toEqual(Map([[a, Map([[b, Map([[c, 10], [d, 2], [e, 20], [f, 30], [g, 40]])]])]]));

What do you think?

@leebyron Thanks for the suggestion! I'll try this strategy tomorrow morning. I definitely agree that we need to protect the performance of the common path. Thanks for reviewing the changes and the performance results.

Out of curiosity, since the perf tests don't hit these Symbol lines, is it true then that the typeof check is what slows us down so drastically?

@abacaphiliac - I believe the node environment does define this method, so my guess is that the slow down is from both the addition of the typeof (probably slight), the calling of the method (no idea how costly) and the resulting call to .concat([]), which could potentially deopt if the vm has a fast-path for Object.keys()?

That all makes perfect sense. It would also make sense to only concat (etc) when there are one or more Symbols. I'll run perf tests against that as well, just so we know. Thanks again.

@leebyron Again, I can't thank you enough for working through this with me. I've spawned a couple additional requests from the work we did together here, about testing and documenting existing functionality and processes.

I tweaked this request once more, but performance is still an issue: feature_perf3.txt

I think I've exhausted all options in this request, so it is probably time to close it. Do you think I should open an issue clearly documenting the desire to mergeDeep object literals with Symbol keys? I can reference this request and the performance concerns so that time won't be wasted.

I think this PR has effectively become that issue, since there's a ton of context here I'm not sure we need to duplicate that into an issue.

Do you think this is mostly a documentation issue? I know that Map({key: "value"}) is by far the most common constructor form, but perhaps we just need to make it more clear that Map([["key", "value"]]) is an encouraged alternative for non-string keys?

@leebyron I think docs could definitely be improved. Mind if I take a crack at it?

The problem I'm trying to solve involves deep merging many object literals, with a mix of Symbol and non-Symbol keys. In order to use this library (for the very helpful mergeDeep function and ImmutableMap type), I'll have to convert my input parameters into a transient data structure, iterable key-value tuples. I'm hoping to avoid owning that transformation.

Would a helper method to transform the "common constructor form" into the "encouraged alternative" interest you? Then the documentation we'll write can reference this helper.

Can you think of another way that we can conditionally trigger Symbol handling? E.g. a new Seq type, or a new merge method?

I think docs could definitely be improved. Mind if I take a crack at it?

Please!

Would a helper method to transform the "common constructor form" into the "encouraged alternative" interest you? Then the documentation we'll write can reference this helper.

I think this alone would be a valuable micro-library/polyfill. Immutable.js is not the only place that uses this constructor format, the ES6 collections do as well, and they have no support for using a {k:"v"} input.

Object.entries was recently introduced to solve this problem.

I believe it is supposed to include Symbols in it's output: https://www.ecma-international.org/ecma-262/8.0/index.html#sec-object.entries
calls: https://www.ecma-international.org/ecma-262/8.0/index.html#sec-enumerableownproperties which calls: https://www.ecma-international.org/ecma-262/8.0/index.html#sec-ordinary-object-internal-methods-and-internal-slots-ownpropertykeys
Which explains that Symbols are included.

Perhaps this is the helper method you're looking for, and we just need to advocate for a good polyfill in the meantime?

Though Chrome's current implementation doesn't seem to agree:

Object.entries({[Symbol()]: 'symbol'})
> []

Ah, I misread. Step 4.a. in "EnumerableOwnProperties" specifically disallows Symbols (and Array indices) from being included in Object.entries(). If anything, this reinforces to me that the current Map constructor behavior is the correct one - that is Symbols should be ignored from enumeration unless explicitly included via a unique method.

https://www.ecma-international.org/ecma-262/8.0/index.html#sec-enumerableownproperties

However the Reflect.ownKeys API supports Symbols and may be a better place to start. It seems like what we want for this case is Reflect.ownEntries, which actually gets a casual mention in the Object.entries proposal: https://github.com/tc39/proposal-object-values-entries#iterators-or-arrays.

We could make a quick polyfill if we can rely on Reflect.ownKeys:

function ownEntries(obj) {
  return Reflect.ownKeys(obj).map(k => [k, obj[k]])
}

So now:

const a = Symbol('a');
const m1 = Map(ownEntries({[a]: "symbol"}));
// Map { Symbol(a): "symbol" } 

@leebyron So much good info! I'll give it a whirl!

@leebyron Sorry about the delay. This is what I came up with. Not sure if you had a "named constructor" in mind, but it felt appropriate especially since I needed to handle nesting with recursion.

Just wanted to say, if you could throw in Symbol support either for the current constructor or for an alternative method for the constructor, I would find it very useful.

For my own purposes, I am willing to accept a performance hit on the initial constructor.

Thanks to Timothy and Lee for their efforts on this so far.

thanks.
Robert LaMarca

@leebyron it's no problem at all. thank you for the feedback. i'll address everything.

@leebyron, ready for review.

The new static method has been renamed and documented. It would be great if you could be really picky about the typings, phrasing, and samples. I'm not 100% sure that I've put everything in its right place. The generated docs look OK to me, but the examples do not run in the browser. All three discussion samples import const { Map } = require('immutable@4.0.0-rc.9') and crash with the following error: TypeError: Map.fromOwnEntries is not a function. Have I done something wrong?

Thank you for pointing out the ambiguity of the return types in my new function. Due to the recursion, I wanted a short-circuit in the event that an object's property was not an object. That logic been moved into a ternary shortcut in the key mapper, which gives our new function a consistent return.

With respect to recursion, I have added a new test case specifically covering it. That behavior/expectation was buried in another test I previous wrote covering mergeDeep between two Map instances with nested Symbol keys.

My desired use case does require recursive handling of nested objects. Now that you have me thinking about it, I'm not sure recursion should be the only (or default) functionality. Would you prefer to keep this API slim and push recursion/traversal back onto the consumer? Would two methods be more appropriate (for example, Map.fromOwnEntries and Map.fromOwnEntriesRecursive)?