Add 'toHierarchy()' method equivalent to 'toArray()' method

[LavaSlider]

I was using nested Sortable elements and needed a way to easily dump the hierarchy. The added method is analogous to 'toArray()' except that it includes both ids of the Sortable element and all its nested lists.

This is only new code, three new options and three new methods so should not change other functionality.

[waynevanson]

Could you please write some tests for this? We shouldn't accept PR's without tests.

I think that instead of recursive arrays, we should have this as recursive object-arrays.

This way the data type will always be the same, instead of the array element being of type string[] | null.

const hierarchy = [
  { value: "a", children: [ { value: "b", children: [ ] } ] },
  { value: "b", children: [ /* we could go on forever... */ ] },
  { value: "c", children: [ ] },
  { value: "d", children: [ ]}
]

[waynevanson]

The package-lock.json file looks like it hasn't been gitted correctly.

We can fix this later. Focus on the comments I mentioned earlier instead.

[owen-m1]

@waynevanson @LavaSlider I wrote the code for what Wayne suggested in the answer to a question on stack overflow:
https://stackoverflow.com/a/56589385/6088312

I also think it would be a less confusing approach.
Also we need to update the README.

[LavaSlider]

I considered using an array of objects but went with the way it is implemented because it is most consistent with the current functionality of 'toArray'. It is also much lighter weight. The array types are either String or Array, no null's.

I am happy to add another method that would loop through the current return and convert it to the structure you describe. What would you suggest the name for that method?

I have seen both structures of return from other Sortable/Draggable packages.

I will work on developing some tests and updating the README file as well.

[owen-m1]

@LavaSlider I think it is less confusing to just use one method, and to me the object with children property makes the most sense for persisting data, because it causes the least confusion.

Sortable is a big library with a lot of users and it isn't a library that doesn't requires much programming experience to use, so inevitably there will be lots of questions over any bit of possible confusion.

[LavaSlider]

I am attempting to write tests but when I run npn run test it only reports FAILED COUNT 1 without giving the test name or the message from the actual assertion. At the end the screen contains:

sortablejs@1.10.2 test /.../Sortable
node ./scripts/test.js

FAILED COUNT 1
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! sortablejs@1.10.2 test: node ./scripts/test.js
npm ERR! Exit status 1
npm ERR!
npm ERR! Failed at the sortablejs@1.10.2 test script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR! /.../.npm/_logs/2020-04-15T21_47_29_550Z-debug.log

I've looked in the -debug.log file in the _logs directory but it does not have the information. Any suggestions?

[waynevanson]

Not sure what to do. I'll write a more helpful test runner soon.

[waynevanson]

@LavaSlider If you commit your test to the repo, we can have a look

[LavaSlider]

Ok, it is your library and you can do what you want but I whole heartedly disagree about the structure and think your statement is inconsistent with how the toArray works.

Users will use what is provided and want their tools to do what they say they do as efficiently and effectively as possible. Changing to objects with children more than quadruples the data size.

To me they seem virtually identical in use. Below is the same function written for each data array format:

toHTML( arr ) {
    let str = "<ul>\n",
        n = arr.length;
    for( let i = 0; i < n; ++i ) {
        str += '<li data-id="' + arr[i] + ">" + fetch( arr[i] ).toString();
        if( i+1 < n && Array.isArray( arr[i+1] ) ) {
            str += "\n" + toHTML( arr[++i] );
        }
        str += "</li>\n";
    }
    str += "</ul>\n";
    return str;
}

toHTML( arr ) {
    let str = "<ul>\n",
        n = arr.length;
    for( let i = 0; i < n; ++i ) {
        str += '<li data-id="' + arr[i].id + ">" + fetch( arr[i].id ).toString();
        if( arr[i].children.length > 0 ) {
            str += "\n" + toHTML( arr[i].children );
        }
        str += "</li>\n";
    }
    str += "</ul>\n";
    return str;
}

To compromise I have merged them into one method that I renamed to getHierarchy() with an optional "mode" parameter and set the default mode to the maximally verbose method with an 'id' and a 'children' property for every node. Mode 1 will strip the 'children' property if there are no children and mode 2 will just return the id strings and arrays as I originally implemented it.

I implemented tests as part of the current nested test suite by putting the stringified version of the returned values into the DOM and comparing that to the expected strings. This will fail if JSON.stringify() changes its output (such as changes in white space, quotes used, etc.) but works now.

I also added the sections to the README.md file describing the operations.

[LavaSlider]

Oops, had forgot to remove a 60 second delay in the test script I had added for figuring out what was going on.

[waynevanson]

I whole heartedly disagree about the structure and think your statement is inconsistent with how the toArray works

toArray is not toHierarchy. If we want to use objects other than arrays, we have the flexibility to.

Sometimes specific use cases should transform the data to a structure they need with a single function of their own.

Changing to objects with children more than quadruples the data size.

I'd rather have more data and a clear way of navigating it than trying to navigate nested arrays.

To compromise I have merged them into one method

I'd recommend that we get the hierarchy using one method only, then transform that data to it's various different 'shapes'. That way if any changes are made in future, there will only have to be one 'getter' function from the DOM.

type Hierarchy = { id: string, children: Hierarchy[] }

function getHierarchy() :Hierarchy 

I will do a review in more detail when I get the time over the next week. I need more time to process the code.