KittyCAD / modeling-app

The KittyCAD modeling app.
https://kittycad.io/modeling-app/download
MIT License
395 stars 33 forks source link

Executor should call `sendModelingCommand` with pathToNode #2944

Open Irev-Dev opened 3 months ago

Irev-Dev commented 3 months ago

We are currently using sourceRange as our pointer to a specific place in the code, and backwards engineering where this is in the AST, the problem with this is the sourceRanges in the ast go stale as the only way to get accurate sourceRanges in the AST is by parse(recast(ast))ing, so after multiple code-mods we have to do this extra step which is an ongoing source of bugs and is inefficient.

pathToNode is a much more AST native pointer into the code.

Will require the executor to know where it is in the tree as it's executing nodes, so that it can pass the path through to sendModelingCommand

jtran commented 3 months ago

I was thinking about this yesterday, and it occurred to me that we modify the AST during normal use, not just the program text. Those modifications can invalidate a path-to-node because the structure of the AST changes.

If we're going through the work to change this, would we be better off giving every AST node an ID, and using that? Then when modifying the AST, new nodes that would have been at the same path can inherit the same ID. Or not. Maybe it moves to a different path. It depends how we use path-to-node, but an ID could serve the same purpose with even more flexibility.

A drawback is that we'd need to maintain a mapping from ID to node.

jtran commented 3 months ago

If we went with IDs, the other metadata we'd need to store is given an ID/node, what is its parent?

Irev-Dev commented 3 months ago

We talked about this as seemed to reach a working consensus, we can move forward with ids, that doesn't preclude pathToNode though, but instead we might use more of a UUID-path.