[Proposal] Refactor resolve fn to be more flexible and extensible

[mattvague]

Problem

Currently, all of the logic for deciding how resolution works lives in the global resolve function. This means that it's impossible to change the resolution behaviour on custom Node subclasses.

Solution

Move node-specific resolution logic from resolve function to each node type class. This has the benefits of

1. Allowing custom resolve behaviour in Node subclasses
2. Improving encapsulation and polymorphism
3. Making things easier to test as each types resolve function can be tested individually

Motivation

In my app, I'd like to implement a custom node type called RandomValueNode that replaces itself with a ConstantNode with a random value when it's tree is resolved, however the current implementation of resolve makes that impossible.

---

@josdejong thoughts on this idea? If you like it I'll go and add unit tests for each node type resolve method and add some docs.

[josdejong]

Thanks, I quite like this approach, splitting the functionality per node class.

Only downside is that this makes all Node classes a bit more fat again, and when you don't need the functionality you still have to load the full classes. An alternative would be to add a callback handler to the resolve function similar to the callback handler that you can pass to Node.toString or so.

@gwhitney do you have any thoughts on this?

[gwhitney]

Well, at first glance, to get the same effect/ability that Matt is after while avoiding the drawbacks of adding code to every Node class that will only be used by clients that want to do resolveing (which may not be that common), couldn't we rather leverage typed-function and proceed as follows:

• Note that in src/function/algebra/resolve.js the workhorse function is primarily just a big switch on the node type, with a fallback for nodes that do not have a specific algorithm. So split this into several smaller functions, one for each node type that has its own resolve approach, and one for that fallback.
• Add each of these functions as separate implementations in the typed-function for resolve, typed with the specific node types. Note that types for each of the specific node types are already in the typed-function type universe for mathjs ordered before the generic Node type, so all of the individual specific implementations will be tried before the generic one.
• Then the client with a custom node type just has to make sure to add that node type to the typed-universe (thankfully because of update to typed-function v3 you can request a newly added type comes before a specific type, so the client can put it before the generic Node type).
• Finally, the client merges a specialized implementation for resolve typed for the custom node type to the built-in resolve with math.import({resolve: myCustomNodeResolution})

I think this approach has a much lighter footprint on existing mathjs code, will all work, and accomplishes everything Matt was aiming for. That said, I haven't actually tried it, so there could be speedbumps I am not seeing... Hope this helps.

[josdejong]

adding code to every Node class that will only be used by clients

Yes, that is an issue indeed, it makes the Node classes more bloated, and that code is only relevant for people using the resolve function.

So, an other approach could indeed be making the resolve function itself extensible. I like Glen's idea of seeing if we can utilize the function being a typed-function, just being able to add another signature for another Node type. Or alternatively we could extend the function with some callback or extra parameter that allows passing additional logic and nodes. What do you think @mattvague?

(Glen I think you mean src/function/algebra/resolve.js instead of src/function/algebra.js in your first bullet point btw)

[gwhitney]

(Glen I think you mean src/function/algebra/resolve.js instead of src/function/algebra.js in your first bullet point btw)

Indeed, fixed.

[mattvague]

So, an other approach could indeed be making the resolve function itself extensible. I like Glen's idea of seeing if we can utilize the function being a typed-function, just being able to add another signature for another Node type. Or alternatively we could extend the function with some callback or extra parameter that allows passing additional logic and nodes. What do you think @mattvague?

@josdejong Yeah those are definitely other options, I'm not stuck on my implementation at all. I guess I'm just still not that familiar with typed-function.

Note that in src/function/algebra/resolve.js the workhorse function is primarily just a big switch on the node type, with a fallback for nodes that do not have a specific algorithm. So split this into several smaller functions, one for each node type that has its own resolve approach, and one for that fallback.

Add each of these functions as separate implementations in the typed-function for resolve, typed with the specific node types. Note that types for each of the specific node types are already in the typed-function type universe for mathjs ordered before the generic Node type, so all of the individual specific implementations will be tried before the generic one.

@gwhitney Could you give me a pseudo-code example of how the resolve function would change with your proposal?

Finally, the client merges a specialized implementation for resolve typed for the custom node type to the built-in resolve with math.import({resolve: myCustomNodeResolution})

Could also you give me a pseudo-code example of what myCustomNodeResolution would look like?

[gwhitney]

Well the best way to describe is to do, so I can go one better and just provide #2801 that implements my suggestion. And the new test I added for the use case you propose should provide a pretty good outline of how to do what you want to do. Let us know if it works for you.

[josdejong]

Thanks Glen for working out a demonstration in #2801. I quite like that the Node classes are not aware of anything related to resolve in that approach. It feels like a proper way to do this on the level of typed-function: define a new Node, and override resolve with an extended version when special behavior is needed. Having to override three different signatures (in the example: 'IntervalNode', 'IntervalNode, Object|Map|null|undefined', and 'IntervalNode, Map|null|undefined, Set' is relatively complex though and can be a barrier for people.

An other idea: we can go with the approach of Matt, but instead of basing it on methods Node.resolve, we can could configure a map with the node name as key and a resolve function as value and attach this map to the function or pass it as argument. Or we can have a handler callback similar to the transform function where you can inspect the node type yourself and resolve it.

@mattvague what are your thoughts?

[gwhitney]

The "overriding three signatures" thing is because typed-function doesn't allow optional arguments. I don't see why this architecture/leveraged use of typed-function should be penalized because of that.

But based on your comment, the implementations can be rearranged so that there are single generic forwarders with undifferentiated Node arguments and only one "workhorse" for each Node subtype. Shall I push a commit with that refactor so you all can look? (Also I can fix the doc error.)

[gwhitney]

OK, I went ahead and rearranged the implementations in #2801 so that to add any new node type, you only have to supply the NewNodeType, Map|undefined|null, Set signature. (It has to be the most elaborate one so that generic forwarders can supply any missing arguments; there is no way to figure out how to handle all three arguments if only given an implementation that doesn't take all of them.) Also, the documentation tests are all passing now. Hopefully that's better?

[josdejong]

😎 nice, these last tweaks in your PR take away my argument against your proposed solution Glen. I think #2801 is a perfectly good solution now, and going for this approach or these alternative ideas I proposed is mostly a matter of personal preference.

@mattvague any feedback?