Document `FunctionNode.name` and add it to Typescript typings
Hi,
I've found myself relying on FunctionNode.name but it's neither in the docs nor in the Typescript typings. This PR would formalize FunctionNode.name as part of the API by adding a name: string field to the Typescript type definitions, and documenting the field in docs/expressions/expression_trees.md
Thanks Josh, I can imagine using .name is handy.
We have to be careful here: in general it is possible that JavaScript minifiers mangle function names, replace it with a short 1 letter to get a smaller bundle. I think because we use named exports the name will stay intact, but I'm not totally sure. It would be good to do a bit of research on whether this can indeed be an issue, and I would love to have a unit test in place to test the .name property on the minified bundle produced by mathjs.
Wanted to see if we could try to get this merged (and off the overfull PR plate). @josdejong , does the latest commit I just pushed include the kind of test of exporting FunctionNode.name that you were interested in?
However, in addition I have a couple of concerns. Why does the documentation of FunctionNode state that the .fn property is read-only? I am able to create FunctionNodes and then assign to their .fn property and use the resulting expressions; nothing seems to go awry. On the other hand, the .name property is explicitly made to be read only. So should I or Josh
- Change the documentation of FunctionNode so that .name is marked read-only and .fn is not?
- Extend the test I just added to verify that JavaScript barfs if you try to assign to .name? Or since it actually appears to be legit to assign to .fn, should we change the set() function for .name to take a string s and replace the .fn property with
new SymbolNode(s)and not barf? (In which case, neither property should be marked read-only in (1), I presume.)
And one more question about the docs that perhaps we should fix while we are there: 3) Although the constructor takes a node or string in the first argument, the .fn property is always a Node, as correctly reflected in the TypeScript typing for FunctionNode in types/index.d.ts. So should the documentation of FunctionNode in docs/expressions/expression_trees.md drop the "| string" annotation in the documentation of the .fn property of a FunctionNode?
@joshhansen, assuming Jos would like some or all of (1)-(3), let me know if you will make the changes or I should. Thanks to both of you.
I've had a look at the .name property of FunctionNode, I think it's safe to use it and basically all we have to do make the documentation and type definitions on par.
There is one small caveat: sometimes the name is empty, like when parsing the expression A[4](x) (get the 4th item from matrix A, then execute that as a function with parameter x). Thinking about it now, it would make more sense to me to return null instead of an empty string in that case, it's an anonymous function.
Why does the documentation of FunctionNode state that the .fn property is read-only?
Hm, yeah, I'm not sure. I think we should remove that. At least .name is readonly, so we should update the docs telling .name is readonly, .fn is not. And you're right about .fn always being a Node and not a string. A few extra unit tests to test reading/writing .fn and .name cannot do harm of course, if it is not yet covered.
So in short: (1)-(3): yes, yes, and yes 😄
OK, that's all clear. So @joshhansen let me know if you will be adding items (1)-(3) above to this PR or if you would like me to (which will probably take a little while, as my plate is fairly full at the moment).
I am willing to do items (1) through (3) above to get this over the finish line.
Thanks Glen