setIn behavior contradicts documentation

[ariesshrimp]

Problem

Using collectable@0.22.0

It looks like C.setIn() tries to interact with values at times when I expect it to be interacting with keys.

See this webpackbin for a demo.

Take special note of map2, which is the operation that appears to throw the type error. Copy-pasted from the README:

import * as C from 'collectable';

const input = {
  foo: 'abc', <--- setIn tries to do stuff to 'abc' when it should be operating on input.foo...
  xyz: [3, [5, 6], 7, 9]
};
const map0 = C.from(input); // <{foo: 'abc', xyz: <[3, [5, 6], 7, 9]>}>
const map1 = C.updateIn(['xyz', 1, 0], n => 4, map0); // <{foo: 'abc', xyz: <[3, [4, 6], 7, 9]>}>
const map2 = C.setIn(['foo', 'bar'], x => 'baz', map1); // <{foo: <{bar: 'baz'}>, xyz: ...>

VM181 dll.js:9431 Uncaught TypeError: Cannot use 'in' operator to search for '@@is-collection' in abc
    at isCollection (VM181 dll.js:9431)
    at isIndexedCollection (VM181 dll.js:9434)
    at setDeep (VM181 dll.js:7644)
    at path.length.__WEBPACK_IMPORTED_MODULE_0__collectable_core__.IndexedCollection.updateEntry (VM181 dll.js:7647)
    at update (VM181 dll.js:9817)
    at HashMapStructure.@@update (VM181 dll.js:3664)
    at Object.updateEntry (VM181 dll.js:9425)
    at setDeep (VM181 dll.js:7647)
    at Object.setIn (VM181 dll.js:7639)
    at Object.<anonymous> (VM182 main.js:96)

Desired Outcome

• [ ] If the documentation is misleading, then the example would be corrected to demonstrate the actual setIn() behavior. This is likely, since there are many calls for documentation throughout the project.

• [ ] If the documentation reflects the intended API correctly, then this is a bug in the implementation of setIn()

[ariesshrimp]

I wanted to go examine how setIn() is used in the tests, but there aren't any tests yet! 😂 this might reflect a good starting point for this issue

[ariesshrimp]

Here lies the setIn()definition, but I'm not sure that's the breaking point.

[axefrog]

Hi Joe, thanks for giving this a go! It's heartening for me to see others starting to see value in it :)

As you may have seen in other issue discussions, there is work to be done as yet, including getting the documentation up to date, and though there are close to a thousand tests so far, some things are still untested, one of those being the deep operations API, which I wrote but have not gotten around to using yet.

Are you looking to contribute to this area? No pressure there, just ascertaining your intentions. If not, I'll look at getting this fixed as soon as I can - I am very welcoming of assistance if you do want to look at this, as I have a lot on my plate. Should you do so, I'll endeavour to review your pull request(s) as quickly as possible, and publish a new package immediately following the merge.

[ariesshrimp]

@axefrog sure, I'd be happy to look into it over the weekend. Thanks for mentioning that there's a lot of existing test coverage. I see now that the tests are written in the package directories for the various modules, not in the top level tests/ directory

I'll start by writing some characterization tests of setIn() and go from there.

btw, I came to this repo from this issue about developing a point-free curried wrapper for immutable.js

[axefrog]

Cool. Let me know if you have any questions. Because the readme files are generally out of date, your best bet is to consult the jsdoc comments where possible. Also, be sure to familiarise yourself with Collectable's mutation API: https://github.com/frptools/collectable/issues/47#issuecomment-314669475

[ariesshrimp]

@axefrog thanks! Actually, I should ask what the the intended behavior of setIn() actually is. I think its intended to work as shown in the documentation (in the copy and pasted code...)

C.setIn(['foo', 'bar'], x => 'baz', {}); // <{foo: <{bar: 'baz'}>}> ✅

[axefrog]

Yeah the first argument is the path, the second is a function that takes the current value at that path and returns the new value at that path. If the returned value is equal to the argument, the collection instance should be returned without any changes applied. If the argument returned is undefined and the nested collection at that location is a map, the entry for the last key in the path should be removed from the map, and if that means the map is now empty, the second-last path key can be removed as well. The third argument is supposed to be a collection instance... I'm not sure why I made it an empty object in the example.

[ariesshrimp]

@axefrog I'm confused by the testing setup, especially this description from the CONTRIBUTING.md

what is your workflow for running tests?

it looks like the test script wants to run against compiled files. but the test scripts don't build anything. the build script builds things! but it doesn't watch files, and the documented section contrasts the test script with the gulp script, so that gives me the impression that i don't need to run the gulp script at the same time as the test script.

looking around at the various package.json's leaves me at a loss.

[SimonMeskens]

running "gulp build --pkg list" will build the package list and run the test suite running "npm run build-all" will build all packages and run the test suite

[ariesshrimp]

@SimonMeskens so you run the commands manually while developing?

[SimonMeskens]

Oh you want watching?

"gulp watch --pkg list"

[SimonMeskens]

For some reason, it seems like there's no global watch command, just running "gulp watch" will not watch all packages at the same time it seems. If there's a way, we'll have to wait for @axefrog

In general though, I never work on more than one package at once

[SimonMeskens]

Also note that if you want the main package to rely on the sub-packages, instead of the published npm packages, you have to use "npm link"

[ariesshrimp]

@axefrog @SimonMeskens So it looks like there are actually two un-related problems with setIn().

1. setDeep() doesn't first check that collection is an object before attempting to validate it using isIndexedCollection():

setIn(['a'], 'b', from({}));
// calls isIndexedCollection(undefined) for 'a'
TypeError: Cannot use 'in' operator to search for '@@is-collection' in undefined

2. Even once that's fixed you see this problem: setIn() sets its target value directly, rather than calling an updater function as demo'd in the docs. In this case the updater function is passed as:

/*
function setDeep(collection: IndexedCollection<any, any>, path: any[], keyidx: number, value: any): IndexedCollection<any, any> {
...
    return keyidx === path.length - 1
      ? IndexedCollection.set(key, value, collection)
      : IndexedCollection.updateEntry(
*/
/* this guy */
(c: any) => setDeep(c, path, keyidx + 1, value), 
// ...
}

notice that value is set naïvely, even though the README suggests that it should be called as a function, presumably with any already defined value as an argument?

const result = setIn(
  ['a'], // path
  v => 'b', // docs show that setIn takes a function
  from({}) // operand
);
unwrap(result) // {a: v => 'b'}. should be {a: 'b'}

So I see 2 as a separate issue that will also break setIn(), but for a different reason than 1. Or, as a confusion between the documentation and the desired API. What is the desired API for setIn()? That's an issue worth opening and exploring.

As to 1, I see it as the cause of the problem described by this issue. I'm not sure where in the callstack is the right place to position the undefined guard. Does it belong up here in setIn()? Or does it belong down in isCollection()? Or somewhere in between?

[axefrog]

I'm going to go ahead and disclaim that this is a set of code I wrote but have not yet had a chance to test it in any capacity, so it does likely have bugs. The behaviour, if working, should be relatively straightforward. When the path does not match what exists already, what exists should be replaced with the best collection type for the path element type (a string would suggest a map key, whereas a number would suggest a list index, unless there is already a map at that location). Returning the same value that already existed at that location should be a no-op. Returning undefined when it was previously defined should indicate deletion if the nested collection type is a map.

Because this was an untested initial implementation, it's possible also that there are ambiguities I hadn't considered. If so, I welcome any discussion on how the API can be improved.

[axefrog]

Regarding watching tests while developing, if you cd into the relevant package directory then run npm run test-dev AFTER starting the VS Code's build task, which is set to tsc-watch mode, then the build task will keep the output files compiled as you change them, and the test-dev script will execute mocha in watch mode against the built outputs, allowing you to edit your code and see the tests re-run a few seconds after you save the file.

[ariesshrimp]

@axefrog cool, i'll open a new issue about settling the second parameter API. i'd also like to add some list tests in for the number implication you described