datavis-tech / graph-data-structure

A graph data structure with topological sort and shortest path algorithms.
MIT License
249 stars 46 forks source link

New major version proposal #81

Open JesusTheHun opened 1 month ago

JesusTheHun commented 1 month ago

New major version proposal

Breaking changes :

Internal changes :

Features :

const g = new Graph<{ id: string }, { type: string }>();

// Good ✅
g.addNode({ id: "abcdef" });
g.setEdgeProperties(source, target, { type: 'foo' });

// Bad ❌ TS Error
g.addNode({ id: 1 });
g.setEdgeProperties(source, target, { label: 'foo' });

const node = g.nodes();
//     ^? { id: string }[]

const props = g.getEdgeProperties(source, target);
//     ^? { type: string }

## Migration

### Serialization
```diff
-const serialized = Graph().serialize();
+const serialized = serializeGraph(graph);

-const graph = Graph().deserialize(serialized);
+const graph = deserializeGraph(serialized);

Algorithms

-const sorted = graph.topologicalSort();
+const sorted = topologicalSort(graph);

-const path = graph.shortestPath();
-const nodes = path;
-const weight = path.weight;
+const { nodes, weight } = shortestPath(graph);
curran commented 1 month ago

Wow, this is a very invasive change! There are so many changes here, it will take me some time to review it all and consider the consequences of each one. I would prefer smaller isolated PRs that make individual changes so that each one can be considered and evaluated individually.

For example, each of the following could be done as separate PRs:

One thing that also comes to mind is that if we go ahead with such invasive changes that require a major version bump, let's tackle https://github.com/datavis-tech/graph-data-structure/issues/18! Ideally the graph functions would not be all in one monolithic import, but rather separate modules that could be imported and used. The migration to a class-based approach is a huge step in that direction.

Perhaps we could adopt a pattern where users of the library can import functions and then bind them to the class somehow? Or maybe they could be functions where the Graph is passed in and the functions access their internals.

In summary, I may ultimately merge in this PR, but I first need to do a thorough review of all the changes, which will take some time. I'm heading out on a trip now and will be back in July. If you'd like to get changes in sooner, please make smaller PRs.

Thanks so much for your contribution!

JesusTheHun commented 1 month ago

There are so many changes here, it will take me some time to review

hi @curran I didn't expect you to merge this right away 😄 but those are the changes I'm going to need to go forward in one of my project, so I thought I would share them.

Note that the breaking changes are clearly identified and contained. It will be easy for users to migrate.

  1. The migration to vitest is pretty straight forward, the tests have been migrated to perform the exact same assertions.
  2. The migration to a class comes with all the TypeScript sugars, that itself is tied to the possibility to reference nodes as objects instead of just string.

I would prefer smaller isolated PRs that make individual changes so that each one can be considered and evaluated individually.

I could have done separate PRs, but that would have induced substantially more work, especially for the TypeScript side of it.

One thing that also comes to mind is that if we go ahead with such invasive changes that require a major version bump, let's tackle https://github.com/datavis-tech/graph-data-structure/issues/18

This PR does not go in the right direction for #18 . Tree-shaking is not possible when using classes. The way to go would be composition :

We would have GraphData, a class that holds the data and allows to manipulate it (addNode, addEdge, etc...). Then functions that take a GraphData as first argument.

import { GraphData, topologicalSort } from 'graph-data-structure';

const graphData = GraphData.deserialize(data);
const sortedNodes = topologicalSort(graphData);

This structure would allow tree-shaking, only the imported classes and functions would be included in the app's bundle. As it is right now, I would say we avoid premature optimizations : the ESM bundle is currently 5kb, which is completely fine.

JesusTheHun commented 3 weeks ago

@curran Now you have composition ;)