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

Do we really need cosmoz-tree ? #2

Open
plequang opened this issue Aug 8, 2017 · 3 comments
Open

Do we really need cosmoz-tree ? #2

plequang opened this issue Aug 8, 2017 · 3 comments
Assignees

Comments

@plequang
Copy link
Contributor

plequang commented Aug 8, 2017

This is not really an issue but rather a place for discussion about the purpose of cosmoz-tree.

cosmoz-tree is not a web components. It is a adapter class used by other Cosmoz web components that act on a hierarchical data structure: cosmoz-treenode, cosmoz-treenode-breadcrum and cosmoz-treenode-navigator.

There are few Javascript tree data structure libraries available, like https://github.com/afiore/arboreal or http://jnuno.com/tree-model-js/.
These libraries relies on a parent->children data model, for example:

{
  "id": 1,
  "name": "root",
  "children": [
    {
      "id": 2,
      "name": "a",
      "children": [
        {
          "id": 3,
          "name": "c",
        }
      ]
    },
    {
      "id": 4,
      "name": "b",
      "children": [
        {
          "id": 5,
          "name": "d",
        }
      ]
    }
  ]
}

In our product, we use 'Materialized paths' to store hierarchies: with each node in a hierarchy, we store a string representing the path of the node in the hierarchy.

Additionally, we apply security on hierarchies, so an end user might only have access to a partial tree, with "holes".
For example, in the above tree, a user might have access only to nodes 3 and 4.
In that case, our data structure looks like the following:

{
  "3": {
    "id": 3,
    "name": "c",
    "pathLocator": "1.2.3",
  },
  "4": {
    "id": 4,
    "name": "b",
	  "pathLocator": "1.4",
	  "children": [
      {
        "id": 5,
        "name": "d",
        "pathLocator": "1.4.5",
      }
    ]
  }
}

So because this might look "unconventional", we use cosmoz-tree as an abstraction layer to a tree data structure. cosmoz-tree is an abstract class, and if you want to use any of the cosmoz-tree* web components, you first need to implement you own cosmoz-tree.

That's for the background.

There are actually several solutions to this tree data structure problem:

  1. use an abstract class like cosmoz-tree,
  2. use a configurable adapter: a class that would accept all kind of tree data structure, and use loads of options to describe this data structure (name of the children property, name of path property if exists, does it contains holes, ...)
  3. force developers to use our own tree representation, letting them convert their data to our model.

If we decide to continue with the abstract cosmoz-tree option, the abstract api MUST not make any assumption on the concrete data structure, especially about the presence of a path locator.

As said in the beginning of the issue, this is an issue for open discussion, so waiting for comments.

@plequang
Copy link
Contributor Author

plequang commented Aug 8, 2017

@nomego @JaySunSyn Could you have a look please ?

@nomego
Copy link
Contributor

nomego commented Aug 8, 2017

Good points!

First of all, we must put our own needs first - enabling it with flexible abstraction layers will always be a secondary priority, so for that reason, a good starting point would be path 3.

But if possible, it is of course of interest to make it as generally usable as possible.

Another thing is that we have gone down this design and road due to some idea of tree structure complexity, manageability, size and performance I'm not sure we will ever encounter.
So one idea is to actually redesign the tree and use an "off-the-shelf" library to handle our tree as well.

Because thing is, a tree structure will in the end need the kind of stuff we're developing now, no matter how you design it. If we call it "pathLocator" and a file system representation calls it "filepath", in essence they solve the same problem of giving a path to find a certain node in the tree structure.

So, right now, for me, it feels most natural to do something in between 1 and 2 - we can design "cosmoz-tree" as an "abstract" class to define what kind of actions we expect to do on a tree structure, and what kind of helpers you would expect to have.
Then we implement that in Cosmoz.DefaultTree, with configurability to say that if you have a tree structure like ours, but use ':' instead of '.' to separate nodes, you can still use our implementation but just configure some "magic" strings to fit your needs.

We could also have a look at method names and properties with wording like pathLocator that feels to implementation-specific and rename them to something more generic along the lines of "path".

In the future, if it feels justifiable, we could replace the tree management with some other library and just add helper methods that are logical for Cosmoz.

I would appreciate some Polymerifcation of this though, to maybe have a "singleton" component to "treeify" a data structure:

<cosmoz-tree model="default" input="[[ dataStructure ]]" tree="{{ tree }}">

I do feel that we have reached a good place now with the consolidation of tree logic that we can use the current setup and slowly move towards a long-term goal, if it should differ from the current approach.

@jokedst can probably help out with some good thoughts as well?

@JaySunSyn
Copy link
Contributor

JaySunSyn commented Aug 17, 2017

Thanks for the background infos @plequang !

Behavior instead or adapter class
Is there a specific reason why did choose to have an adapter class instead of a polymer behavior?

Tree data input type
I'd really like to see e.g. a cosmoz-treenode-navigator to have standard data type as an input property e.g. data instead of expecting a certain object class with custom methods.

If we then use a behavior, we could directly work with data.

If we want to keep for some reason the adapter class, we could set an additional property _tree.

If we of some reason do not want to change that, I think @nomego's suggestion to create a cosmoz-tree element which outputs our tree class is the least we should do.

Tree reperesentation
I'm here with @nomego and point 3. I think it's fine to force developers to use our own tree representation. We could even expose a function like formatNode() to help developers convert their tree representation into ours on a "node level".

What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants