V11 with typed-function@3

[josdejong]

This PR on branch v11_on_v3a replaces the v11 branch, and updates it to use [email protected].

It contains all the commits of all PR's listed in #2476

[josdejong]

@gwhitney this looks very good 😎 . You've done a lot of tiny little improvements, to many to list them all here, thanks!

Some small remarks:

1. The extraction of createCompareUnits and createTrigUnit is nifty, it resolves a lot of redundant code! I think it can be optimized though: right now we first create a typed-function 'compareUnits' containing a signature Unit, Unit, and then strip this signature the typed-function directly to attach it to an other typed-function like compare.js. So, creating this temporary typed-function compareUnits is redundant, and we could directly attach this signature to the final function instead? Same with createTrigUnit. Or do you perfer the current solution because of better readability and being better contained?
2. Can you add an explanatory comment on top of the regex of identifier explaining what it matches, and can you add a couple of unit tests for the new type identifier?
3. Can you write a unit test for the new method Unit.valType(). Also, can you rename it to Unit.valueType()?
4. Can you write a unit test for the new boolean support of hasNumericValue?
5. I see you've refactored the typeOf function. I do like this on the one hand (it probably indeed improves performance), but I would like to think it through a bit more:
   • When minifiying&bundling JavaScript, names may be mangled. For example right now I get the following when executing in the JS console of my browser on https://mathjs.org:

     console.log(math.unit('5cm').constructor.name) // "S" and not "Unit"

     Instead of relying on .constructor.name, we could use .constructor.prototype.type I think, that property is added by mathjs itself.
   • The new solution also returns "something" when you pass an unknown class to it. This is a pro and a con. A con as in: we don't control this and cannot guarantee anything about it. Do you have thoughts about that?
   • typeOf(...) can now be inconsistent with say isSymbolNode(...) since they don't use the same logic.
   • I have to double check if this can have any security implications. I don't think so but want to be a bit careful with that.
   • Changing "Function" to "function" is a breaking change, we should not forget to describe that in the changelog (I don't have preference for one or the other myself)
6. Can you explain your comment // I don't understand the Node class hierarchy? I would like to understand what you mean.
7. I've added a commit with a bit more of IE11 of cleanup of config and polyfills. I'll also try out if we can upgrade to the latest versions of webpack, karma, and mocha now we got rid of IE11, but will do that in a separate branch.

[gwhitney]

Thanks for the excellent feedback. I am now on a post-finals trip to some national parks and so will only be able to get to this a bit until next week.

[gwhitney]

On 1, createCompareUnit and createTrigUnit do not construct any typed function, just a plain object with a key which is the respective Unit signature, which is then incorporated as one signature of various typed functions in the respective category. I don't see any way to further optimize this but if you have an idea how please justcley me know.

[gwhitney]

On 2-4 I will certainly do as requested as soon as I can get to it. Responding to 5-6 will have to wait until I have more time to focus on this. And 7 I don't think there's any action item on my part, sounds good.

[josdejong]

I am now on a post-finals trip to some national parks and so will only be able to get to this a bit until next week.

👍 enjoy

[josdejong]

On 1, createCompareUnit and createTrigUnit do not construct any typed function, just a plain object

Ahh, now I see. You're right, there is nothing to optimize there, my fault.

[gwhitney]

OK, have rebased this branch on develop with all of its changes to the typescript stuff, will look at items (5) and (6) and implement whatever's needed as soon as I can.

[josdejong]

Thanks 😎 . Was it difficult to get the branch up to date again?

[josdejong]

OK, have rebased this branch on develop with all of its changes to the typescript stuff, will look at items (5) and (6) and implement whatever's needed as soon as I can.

@gwhitney is this PR still on your radar?

[gwhitney]

Absolutely! Have just been trying to get through an onslaught of other responsibilities. Should get back to this soon.

[josdejong]

👍 no problem, take it easy. Just wanted to make sure we don't forget about it.

[josdejong]

@gwhitney do you expect to have time to finish this PR on short term? If not, I will see if I can finish it up next week (shouldn't be too much work, you took care of about everything).

[gwhitney]

I do think I can get to it next week. That is my plan. I am working right now on a change to the "inkscape-silhouette" package that I promised them long ago I would do when they changed the name of the primary branch away from "master" and they have finally done that so I feel owe them. As soon as that's done, it's back to mathjs.

[josdejong]

That is good news, thanks for the update! Take it easy.

[gwhitney]

OK, back to being (re)based on develop. I will see if I can stay afloat this time!

[gwhitney]

On review item (3): renaming valType => valueType. But there are already numerous tests of it in test/unit-tests/type/unit/Unit.test.js.

[gwhitney]

Review item (6) was not really an action item but a question for me. The point of the comment in src/utils/is.js that "I don't understand the Node class hierarchy" is that it shouldn't be necessary to have the special case for isNode; all of the different Node types (are/ought to be) "official" JavaScript classes and as such, there should be a way to compute their types "for free" without relying on some property that we set up, i.e., based on the name of their constructor. And if they were all defined with current class syntax, I think that would be the case. But they are defined with an older pattern that I am not familiar with, and it seems like the constructor name is just "Node" for all the types. So rather than trying to figure out a "direct" way to extract the type, I fell back to running the isNode() test and then using the particular data item that the hierarchy set up. But for performance I had wanted to minimize the number of cases and tests that were run on all objects, and maximize the situations in which the type can just be directly extracted. But I punted on that for the Node hierarchy, potentially slowing things down for other types, hence the comment.

[gwhitney]

Ok that just leaves review item 5, definitely the most complicated of all of them. I will attack tomorrow.

[gwhitney]

One question on name mangling though: did you try your test on the code in this branch? I added a number of constructs like

Object.defineProperty(Unit, 'name', { value: 'Unit' })

that supposedly should disallow such name mangling. They seem to work for me but I agree if they are not reliable we shouldn't use that property. So it would be good to know, if you have a chance to check. Thanks.

[gwhitney]

5. I see you've refactored the typeOf function. I do like this on the one hand (it probably indeed improves performance), but I would like to think it through a bit more:
   • When minifiying&bundling JavaScript, names may be mangled. For example right now I get the following when executing in the JS console of my browser on https://mathjs.org:

     console.log(math.unit('5cm').constructor.name) // "S" and not "Unit"

     Instead of relying on .constructor.name, we could use .constructor.prototype.type I think, that property is added by mathjs itself.

As I mentioned in my last question, I believe that I have added code to prevent such name mangling, and I believe it is properly tested by the generated/node tests.

* The new solution also returns "something" when you pass an unknown class to it. This is a pro and a con. A con as in: we don't control this and cannot guarantee anything about it. Do you have thoughts about that?

Well, most of the uses of typeOf are for messaging purposes, where I think doing "something" on non-mathjs types is completely positive. The next biggest use is for function dispatch on Units (the motivation for the rewrite), where we control the types that go in in the first place, so I don't see a worry. Then there's a similar use in the matrix classes, also not a concern. It's used "directly" in the numeric() function, which I don't really understand the use case for (also seems to overlap with math.typed.convert()) but presuming we can't get rid of it I think it could easily be written without typeOf, but moreover, numeric() only proceeds if the result of typeOf is on a prescribed list. So I don't in the end see a worry there. So overall, I don't see any significant concern about typeOf returning "fiddleydiddley" for some non-mathjs entity... If you have a more specific scenario of concern, would be happy to consider more.

* `typeOf(...)` can now be inconsistent with say `isSymbolNode(...)` since they don't use the same logic.

This is admittedly not ideal. Actually the Node-family tests currently only rely on the Node classes keeping the ".type" property in line with the .isXxxNode properties, which presumably they do. And the other .isBlahblah tests could be changed to run through typeOf, or left alone; happy to hear your thoughts. Whether or note we change implementations to be more consistent, we do have tests of all of the .isBlahblah predicates and that typeOf returns the expected thing on an example instance of each entity, so any discrepancies are likely to be caught... so I am willing to live with this if you are, either with doing what's feasible to align implementations or not. Let me know what you'd like.

* I have to double check if this can have any security implications. I don't think so but want to be a bit careful with that.

Basically, I think my analysis of the point about returning "something" addresses this, too. Yes, a foreign class could spoof any mathjs type, but that was already true with the old typeOf, and none of the uses of typeOf make such spoofing seem particularly harmful.

* Changing `"Function"` to `"function"` is a breaking change, we should not forget to describe that in the changelog (I don't have preference for one or the other myself)

Well, it was the only non-object case where typeOf didn't match typeof, which seemed counterintuitive, and it was not much entrenched, so seems worth changing for consistency.

OK, just let me know what remaining action items you have for me based on your review, and let's get this merged :)

[josdejong]

Good to have you back, thanks for the updates :)

(1) about constructor.name

Ahh, now I see what you did, by setting Object.defineProperty(Unit, 'name', { value: 'Unit' }) for example. Yes that indeed works, nice, it simplifies typeOf a lot.

So overall, I don't see any significant concern about typeOf returning "fiddleydiddley" for some non-mathjs entity

Thanks. I think you're right and we do not need to worry. Ok let's go for the new constructor.name approach.

(2) about Node class hierarchy

About the node class hierarchy: now I understand the confusion. The if (isNode(x)) return x.type is just a lazy way such that we do not need to write out a check for every class (the old way) like:

if (isSymbolNode(x)) return 'SymbolNode'
if (isOperatorNode(x)) return 'OperatorNode'
...

It's just utilizing the fact that if a class instance is a Node, then we know that it has a .type property holding the name so we can just return that directly. There is no hierarchy things going on there.

With your new approach, I think the line if (isNode(x)) return x.type should be removed from typeOf, and instead, every Node class (like SymbolNode), must also get a fixed .constructor.name, just like Unit and the other classes. Maybe we should even attach a fixed .constructor.name to BigNumber too to remove that special case.

(3) about inconsistency between say isSymbolNode and typeOf:

I think we do not need to worry too much here, this code isn't changed often and is well unit tested. I like the idea of letting the isXXXNode functions simply call typeOf(), but I expect that it would be less performant. Ok just leave it as it is right now, we can always refactor internally if the need arises.

(4) last action points:

1. remove if (isNode(x)) return x.type from typeOf and give all Node classes a fixed constructor.name too.
2. not sure, do we want to give Decimal.js class a fixed .constructor.name = 'BigNumber' too? It makes sense to me but it also feels ugly to override this name. Currently it's mangled in the minified bundle, so not usable anyway, so I think it will not do harm
3. I think it would be good to add some unit tests to test/node-tests/browser.test.js, where we can load the bundled+minified file to make sure typeOf works also after minifying.

[gwhitney]

Thanks for the feedback!

1. I will try to do this (as I said, I don't really understand the sort-of "by hand" class definition style of the Node hierarchy ;-)
2. Decimal and BigNumber are third-party libraries, right? It doesn't feel right to be modifying them. If you'd like to file an issue upstream with them, feel free, but I'd prefer depending on their existing APIs rather than modifying class internals in the meantime. One special case isn't crushing to typeOf's performance. If you get BigNumber to conform in the future, we can always eliminate the case...
3. I thought I did this but I will check and add the tests if not or extend them if I didn't cover all the cases, esp Nodes and BigNumbers.

[josdejong]

1. 😂
2. yeah ok. Let's leave the BigNumber check as it is then 👍
3. 👍 could be they are already there, I didn't see them but maybe I overlooked this

[gwhitney]

I know you found my response to (1) humorous, but really, I am very confused with all the prototypes floating around in the Node class hierarchy and explicitly setting the prototypes and the properties on them. Is there a reason mathjs can't use current class definition syntax? Is it OK if I try?

[josdejong]

Sorry. I think that I didn't fully understand you. The "by hand" class definition with a function constructor and then methods and props under .prototype was just the normal way of doing "classes" in JavaScript before class existed. The only reason it's written like that is that class didn't exist when I created the code. It would be very nice to rewrite it to class indeed, though in the end it's mostly eye-candy I think, but a good improvement. It may be good to address it separately from this PR to prevent this PR from never finishing.

[josdejong]

Maybe this article helps (it's no magic, you just have to know the notation): https://medium.com/@parsyval/javascript-prototype-vs-class-a7015d5473b

[gwhitney]

Yeah, well, I did warn you. Despite numerous tries, I couldn't figure out where/how to assign properties in the derived Node classes to get the "standard" typeOf implementation to produce the correct result, so I just went ahead and converted to class syntax and as expected typeOf() then works perfectly on all of the Node classes. It makes the diff in the latest commit huge though, I am afraid.

As for testing, it turns out that it's the doc test on the documentation of the typeOf function that was checking a lot of cases (hurray for doc testing) so I just went ahead and extended that documentation page to include all of the relevant types. And note
that the doc tests are all done in node on the resulting bundle, so it constitutes a node test as desired. However, I am not sure that will match the browser tests, so looking forward to the output of the current round of tests on GitHub. Once those are all passing, I will rebase for the umpteenth time...

[gwhitney]

OK, fortunately the rebase had no conflicts and it is force-pushed and passes all tests. Speaking of tests, has mathjs dropped Node 12 support? If so, I see in the commit history of this PR that at some point we switched options && options.implicit to options?.implicit and then we switched back because the latter broke on Node 12. So should I now switch those occurrences back to options?.implicit ??

Thanks for letting me know. If not, I hope this can be merged, it's been quite the saga (sorry that several of my other projects and the end of the semester really dragged it out).

[gwhitney]

Oops I see we have not yet merged the no breaking chain parameters PR into this one yet. I guess there is one remaining review comment in #2559. I will rebase that PR and address that comment. Definitely want to get this done with.

[gwhitney]

OK, #2559 is back in good shape and should be merged here before this is merged into develop. Other than the question of implementing #2617 (which I am personally against) I believe everything is ready for merge into develop and release of v11. If you need help writing a release notes to facilitate that -- this is definitely the most major change in a while -- just let me know and I am happy to help.

[josdejong]

Thanks, let's go all in then with the class rewrite 😄

I couldn't figure out where/how to assign properties in the derived Node classes to get the "standard" typeOf implementation to produce the correct result

I think you figured that out before: in Unit.js for example, where you added the first two lines of to make typeOf work:

Object.defineProperty(Unit, 'name', { value: 'Unit' })
Unit.prototype.constructor = Unit
Unit.prototype.type = 'Unit'
Unit.prototype.isUnit = true

With classes we can write it as follows:

static name = 'OperatorNode'
get type () { return 'OperatorNode' }
get isOperatorNode () { return true }

I added unit tests to test typeOf on the bundle, they are currently failing, and can be fixed with the above definition of name. (on a side note: this should not conflict with the .name property that SymbolNode and FunctionNode instances use too, though it is a bit confusing). Can you make the unit tests pass by adding static, fixed class names?

[josdejong]

Thanks Glen, I think everything is ready, I will press the merge button now 🎉 . Thanks again for all your efforts!