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

Document FunctionNode.name and add it to Typescript typings #2395

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

joshhansen
Copy link
Contributor

@joshhansen joshhansen commented Jan 26, 2022

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

@joshhansen joshhansen marked this pull request as ready for review January 26, 2022 08:22
@joshhansen joshhansen changed the title Add missing FunctionNode.name field to Typescript types Document FunctionNode.name and add it to Typescript typings Jan 26, 2022
@josdejong
Copy link
Owner

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.

@gwhitney
Copy link
Collaborator

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

  1. Change the documentation of FunctionNode so that .name is marked read-only and .fn is not?
  2. 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.

@josdejong
Copy link
Owner

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 😄

@gwhitney
Copy link
Collaborator

gwhitney commented Mar 2, 2022

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).

@gwhitney
Copy link
Collaborator

I am willing to do items (1) through (3) above to get this over the finish line.

@josdejong
Copy link
Owner

Thanks Glen

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

Successfully merging this pull request may close these issues.

None yet

3 participants