Should OrderedMap#setIn create a new OrderedMap (rather than Map) if a key does not exist?

[tomduncalf]

I'm new to Immutable JS and not an expert on immutable programming by any means so I may be confused here, but I would expect calling OrderedMap#setIn to create a new OrderedMap at the given keyPath if a key does not exist - however, according to the documentation and my testing, it instead creates a plain Map.

(From the docs: "Returns a new Map having set value at this keyPath. If any keys in keyPath do not exist, a new immutable Map will be created at that key.")

My concern here is that the order of key/value pairs within these child Maps (that are created when calling setIn with a non-existent key path) will not be preserved in all cases, which is important for my use case - or is this a non-issue as the top level data structure is an OrderedMap?

[leebyron]

I'm not sure I understand what issue you've run into. Could you maybe provide some sample code and explain how you expected it to work and how this library behaved differently?

[tomduncalf]

Sure... I'm happy to explain what I am trying to do in more detail if it's helpful, but for now I've created a fiddle which shows basically what I am seeing in a simplified form: http://jsfiddle.net/8hdbrhdd/1/

Note that this has not caused me any actual issues so far (except when I was trying to construct an "expected" OrderedMap object to compare the actual result's equality to in a Jasmine test, which is how I came across this), but my concern is that the child maps I am constructing using setIn() are not guaranteed to maintain the iteration order of their entries as they were set(), as they are Map objects rather than OrderedMap, and for my application, the order of entries is important (I am constructing this map one entry at a time while looping over lines of an input file).

Would this be the case, or have I misunderstood something? As long as the order of all my nested maps is preserved, I'm not concerned with the implementation - however I would also be interested to know how you would suggest going about comparing a (Ordered)Map returned from a function with the expected result in a unit test - at the minute, I am manually constructing the "expected" Map object, but another option would be to just compare the JSON version of each...

Hope this makes some sense, I am new to the library so please let me know if not and I'll endeavour to explain better :)

[tomduncalf]

Sorry, wrong Fiddle, one sec

[tomduncalf]

Correct URL: http://jsfiddle.net/8hdbrhdd/1/

[tomduncalf]

Actually, no harm in posting my actual code either I guess, feel free to ignore if not useful :)

The idea is that it can take an input like:

[Section 2]
[[Subsection 1]]
Key 3 = Value 3
[Section 1]
[[Subsection 1]]
Key 2 = Value 2
Key 1 = Value 1

and convert it into a nested data structure which maintains the order of the entries in the file

import splitLines from 'split-lines';
import im from 'immutable';

class ConfigParser {
    constructor() {
        this.SECTION_REGEX = /^\s*(\[+)([^\]]*)\]+$/;
    }

    parse(file) {
        // The parsed config (as nested OrderedMap objects)
        var parsed = im.OrderedMap();
        // Keep track of the current section we are in as we read through the file
        // as an array of keys, e.g. ['Section 1', 'Sub Section 2']
        var sectionPath = im.List();

        splitLines(file).forEach(line => {
            // Check if the current line is the start of a (sub-)section
            var sectionMatches = line.match(this.SECTION_REGEX);
            if(sectionMatches) {
                // If so, update the section path accordingly
                var sectionDepth = sectionMatches[1].length - 1;
                sectionPath = sectionPath.slice(0, sectionDepth);
                sectionPath = sectionPath.set(sectionDepth, sectionMatches[2]);
            } else {
                // Otherwise check if this is a config setting, and if so, store it as a key/value
                // in the current section
                var split = line.split('=');
                if(split.length > 1) {
                    parsed = parsed.setIn(sectionPath.push(split[0].trim()), split[1].trim());
                }
            }
        });

        return parsed;
    }
}

export default new ConfigParser();

[tomduncalf]

And an extract of the corresponding test code that made me notice this:

...
  it("should parse a config with one level of sections", () => {
        var result = cfg.parse(`key1 = value1
[section1]
s1k1 = s1v1`);

        var expected = im.OrderedMap({
            "key1": "value1",
            "section1": im.OrderedMap([[
                "s1k1", "s1v1"
            ]]),
        });

        // This fails because the child is actually a Map
        expect(im.is(result, expected)).toBe(true);
    });
...

[tomduncalf]

Any thoughts on this? I've not noticed any problems with child maps being out of order in my work so far, but not sure if I have just got lucky so far, or if I can depend on this order

[leebyron]

Hey Tom, you certainly shouldn't depend on that order. For very small sets of entires, Maps appear to have the same ordering as OrderedMaps, but once you go above a certain size, the ordering can change.

I would suggest explicitly creating the OrderedMaps at each depth if you need to guarantee ordering.

I like this suggestion, but I'll need to think about any externallies.

[tomduncalf]

Great, thanks for confirming that Lee.

Explicitly creating the OrderedMaps is definitely an option, but would make the code a bit more complex - in fact, what made me try out ImmutableJs for this project was the ability to replace several lines of code with one setIn ;)

I might look at patching the OrderedMap to create OrderedMaps at each depth for my purposes, if I do so I will raise a PR and let you decide if it's a good idea or not :)

Cheers,
Tom

[leebyron]

Sounds great

[jardakotesovec]

+1

Thank you for reporting this issue and appreciate your patience. We've notified the core team for an update on this issue. We're looking for a response within the next 30 days or the issue may be closed.

[elmerbulthuis]

+1

[darklrd]

+1