-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
base: develop
Are you sure you want to change the base?
Conversation
FunctionNode.name
and add it to Typescript typings
Thanks Josh, I can imagine using 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 |
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
And one more question about the docs that perhaps we should fix while we are there: @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 There is one small caveat: sometimes the name is empty, like when parsing the expression
Hm, yeah, I'm not sure. I think we should remove that. At least 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 |
Hi,
I've found myself relying on
FunctionNode.name
but it's neither in the docs nor in the Typescript typings. This PR would formalizeFunctionNode.name
as part of the API by adding aname: string
field to the Typescript type definitions, and documenting the field in docs/expressions/expression_trees.md