Add checks for type declarations

[samestep]

This PR adds a test:types script which uses ts-node to check that types/index.ts works (it didn't prior to this PR), and modifies test:all to call test:types so that it gets run in CI. It also makes a couple modifications to types/index.d.ts, partially to fix errors in types/index.ts, and partially to improve usability of the add and subtract functions (see below).

---

(original PR description)

Currently the return types for add and subtract are just MathType when they're passed a Matrix, necessitating type assertions or runtime checks if one wants to give TypeScript a Matrix back. This PR adds overloads to those functions to avoid this issue.

(At first I wanted to try to submit a PR with a generic solution to this issue across the board, but I wasn't sure the right way to do it since I still don't quite understand how the internal implementation of math.js works, so I'm just submitting a PR for these couple simple cases; perhaps the maintainers of this repo have some idea of how to solve this more generally?)

[gwhitney]

Thanks @samestep for this contribution. We would definitely like to get the TypeScript typing of math.js as precise as possible. I agree it's worth taking a moment to see if we can make a more general improvement to the type of issue you raise before going ahead with just these two specific fixes. So, when you say you were looking for something that would fix this sort of issue "across the board," did you mean understanding that the result of matrix operations were matrices for a larger collection of operations, or meaning that encoding the fact that say, basic arithmetic operations preserve types for a wider collection of types, or really both of these things?

Another item that could really help get your changes in is that currently it does not seem that math.js has any testing for the type annotations in types/index.d.ts -- for example, making sure that typescript can make appropriate type inferences based on mathjs computations. If you have any suggestions as to how to perform such testing, or better yet can add a commit to your PR with just an initial pass at adding tests that check type inference in the cases of interest to you (that could in the future be extended to more cases), it would be extraordinarily helpful -- and it would assist in pursuing more general solutions, since having tests in place would ensure the more general solutions don't break any of the existing type declarations.

Thanks so much for your thoughts/assistance and rest assured we will get at least some solution to your situation into mathjs soon. Looking forward to your further feedback.

[samestep]

@gwhitney Thanks so much for the quick and detailed response!

So, when you say you were looking for something that would fix this sort of issue "across the board," did you mean understanding that the result of matrix operations were matrices for a larger collection of operations, or meaning that encoding the fact that say, basic arithmetic operations preserve types for a wider collection of types, or really both of these things?

Yes, both of those things. Initially I was thinking of something like this:

add<T extends MathType>(x: T, y: T): T;

But obviously that doesn't work because it doesn't allow you to add, for instance, a real number and a complex number (which I assume math.js allows?). So maybe it should actually be these two overloads:

add<T extends MathType>(x: T, y: T): T;
add(x: MathType, y: MathType): MathType;

That doesn't feel "optimal", but maybe that doesn't matter too much? But I didn't do that in this PR because I didn't see this pattern for any of the existing functions. For instance:

multiply<T extends Matrix | MathArray>(x: T, y: MathType): T;
multiply(x: Unit, y: Unit): Unit;
multiply(x: number, y: number): number;
multiply(x: MathType, y: MathType): MathType;

Part of my difficulty is that I don't know exactly what are the boundaries of math.js's flexibility (for instance, the return types of identity and zeros depend on the value of config.matrix, although that can't be modified on the global math.js object anyway afaict...). This ties well into the other thing you mentioned:

Another item that could really help get your changes in is that currently it does not seem that math.js has any testing for the type annotations in types/index.d.ts -- for example, making sure that typescript can make appropriate type inferences based on mathjs computations. If you have any suggestions as to how to perform such testing, or better yet can add a commit to your PR with just an initial pass at adding tests that check type inference in the cases of interest to you (that could in the future be extended to more cases), it would be extraordinarily helpful -- and it would assist in pursuing more general solutions, since having tests in place would ensure the more general solutions don't break any of the existing type declarations.

This sounds awesome! I would be more than happy to help with this, whether that means autogenerating types/index.d.ts (is that possible? it seems like it would be, using the signatures from typed-function) or (probably better for the short term) simply testing the static type inference.

I'll come back to this over the weekend; from your expertise on the project, what do you think is the best goal to shoot for here?

[samestep]

Actually, thinking about it more, I realize that we'd probably want to test the typedefs regardless of whether we autogenerate them, so in that case autogenerating them would probably be overkill. Anyways, I'll come back to this soon.

[gwhitney]

One difficulty here is that although I do work on some projects that use TypeScript as the primary language, I am by no means an expert, especially not on TypeScript-JavaScript interoperation. So for example, I think one no-brainer test to add would be to ensure that types/index.ts runs without error and produces the expected output. But I am not even clear how to run that file in the context of a clone of the repo:

> npx ts-node types/index.ts
TypeError [ERR_UNKNOWN_FILE_EXTENSION]: Unknown file extension ".ts" for /home/glen/code/mathjs/types/index.ts

And then even if I could run it, I don't understand how it could work, given that create exported from mathjs does not seem to be typed anywhere at all....

Anyhow, those items aside (and any pointers you might have are welcome!), I do think that getting some TypeScript interoperability and typing tests into the mathjs test suite is the key to moving forward in anything other than tiny incremental steps -- we definitely want to minimize the chances that we break any existing use of mathjs from TypeScript going on out there.

So if you're willing to give initial tests a stab, great!

Once some reasonable testing is in place, I am happy to try to go somewhere more ambitious with the typing itself. Auto-generation from the "typed" declaration is by no means out of the question -- it would be really nice to have one less large, complex piece to maintain by hand. Or if we can't get that to work, then we may just have to continue with a hand-maintained set of type declarations, but try to "optimize" the typing as you point out. And I agree that in any case, we want to capture that add(x:T, y:T) is of type T for a reasonable range of T, so we will definitely adopt a solution that does that, and then if we can manage to also get such inferences as add(x: number, y:complex) is complex, as is add(x: complex, y: number), that would be great -- not to mention the fact that it is perfectly legal to call add on any number of arguments!

Looking forward to your thoughts/initial commits on TypeScript testing.

[gwhitney]

(One final comment at the moment: one tricky bit concerning more exact TypeScript typing, whether or not we try to autogenerate or hand-optimize, is capturing the typed argument conversion rules as laid out in typed.conversions in src/core/function/typed.js and used in the typed-function package which is separate from mathjs itself. Frankly, the parameter matching rules in the presence of conversions in the typed-function package is pretty complicated, so I think likely the only plausible approach to auto-generating is to add a facility to typed-function that provides a TypeScript typing for an instance of a typed function, based on the typed declaration and the conversions in effect. It seems to me that would be a natural addition to typed-function, but I totally understand if that's not a direction you're interested in heading at the moment. If we just get some checks in place, and do some optimizing, and fix your particular points of concern as captured in this initial commit, we will have accomplished much that is worthwhile!)

[samestep]

@gwhitney I pushed a commit to check types/index.ts.

One difficulty here is that although I do work on some projects that use TypeScript as the primary language, I am by no means an expert, especially not on TypeScript-JavaScript interoperation. So for example, I think one no-brainer test to add would be to ensure that types/index.ts runs without error and produces the expected output. But I am not even clear how to run that file in the context of a clone of the repo:

> npx ts-node types/index.ts
TypeError [ERR_UNKNOWN_FILE_EXTENSION]: Unknown file extension ".ts" for /home/glen/code/mathjs/types/index.ts

Yeah, I ran into the same thing. I ended up following the advice from this SO answer, but I'm not sure whether that's actually the right thing to do in this case; I'm guessing you had "type": "module" in package.json for a reason? It does fix the ts-node issue, though.

Also: should I be doing the typechecking-infrastructure stuff and the add/subtract stuff in separate PRs? Currently they're both in this same branch, but given that the discussion in this thread is more focused on the former, let me know if you'd like me to move the latter into a separate PR (probably to merge after this is done, as you suggested).

[gwhitney]

Sam, thank you so much, this is a great start! At this point I have to say we will have to wait for @josdejong to review this PR before it can go in, if changing the type: 'module' in package.json is necessary, because I am not (yet) well-steeped enough in the mathjs package to know why that's there and what might or might not break if it is removed. Is there a workaround, like maybe a flag to ts-node, maybe one that points to a different config file, that prevents having to change this global declaration? That would make adoption more painless.

In the meantime, here's my feedback, which of course Jos may want to amend:

1. I think that rather than touching the github workflows, it would be better to make the TypeScript testing one of the actions that build-and-test does, just like test:node currently is. That's less disruption to the current structure, and eliminates changes to one file.
2. I agree that rather than a formal test harness for TypeScript, since this project is not so much about TypeScript, it's fine to do ad-hoc testing just by running types/index.ts. In that spirit, the current types/index.ts has lots of comments indicating what the results of the operations should be, but there's nothing that verifies that what the comments say actually turns out to be the case. Is there an easy way to include an assertion scheme into the TypeScript file so that those comments can be converted to bits of code that will actually fail if the results are not as expected? (I am not saying parse the comments, just to have a facility so that any of the comments could be converted into a check, and do the conversion on at least some of them as a start, with some kind of intelligible output in case of failure.) Is it possible just to import assert into index.ts (even though assert is a JavaScript library) and change comments into assertions?
3. And if running types/index.ts comprises the TypeScript testing that we are doing, then it seems to me important that it encompasses the type inferencing that we want to enable/ensure. To do that in an comprehensive way is I am sure beyond the scope of this PR (certainly we do not want to make this too big a chore for you!) but since the original point of your submission was to ensure that TypeScript knows that the outcome of adding two Matrix objects is a Matrix, it should be the case at least that types/index.ts will only compile/run without error if that inference can be made. Is that the case for the current types/index.ts? If not, I'd recommend that you add a section devoted to type inference testing, commented as such, which does test that inference (in the usual spirit of adding tests for issues that you fix). (You may want to add a general comment at the top of index.ts that the file serves a dual purpose as TypeScript examples and testing.) Then future PRs can build on that kernel of type inference testing.
4. Finally, I think it is fine to include the "add/subtract" improvements that you want in this PR -- they are minor, and they provide a nice example for extending types/index.ts to encompass type inferencing. (On that topic, did you try just

add<T extends MathType>(x: T, y: T): T;

that you mention above, in place of the current typing and your addition? It seems to me that it might work for adding a number and a Complex, since doesn't number | Complex extend MathType, so that there is an add(x: number|Complex, y: number|Complex): number|Complex instance of this template, so that TypeScript would allow the addition and know that the result is of type number|Complex (which is technically correct)? I am not really sure whether TypeScript can do this, but it certainly seems worth a try.)

Thank you so much for working together with us on this!

[samestep]

@gwhitney

Sam, thank you so much, this is a great start! At this point I have to say we will have to wait for @josdejong to review this PR before it can go in, if changing the type: 'module' in package.json is necessary, because I am not (yet) well-steeped enough in the mathjs package to know why that's there and what might or might not break if it is removed.

Yep, I totally agree that change to package.json shouldn't be merged without careful review by someone who understands what's going on. I suspect it's not correct, because now every time I run npm install in the repo, it tries to make this change to package-lock.json, which seems concerning:

diff --git a/package-lock.json b/package-lock.json
index 92fd19501..ffb86f238 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -5,7 +5,6 @@
   "requires": true,
   "packages": {
     "": {
-      "name": "mathjs",
       "version": "10.1.1",
       "license": "Apache-2.0",
       "dependencies": {

Is there a workaround, like maybe a flag to ts-node, maybe one that points to a different config file, that prevents having to change this global declaration? That would make adoption more painless.

It's possible; the answers and comments on that SO page I linked seemed all over the place, so I really don't know. You mentioned that you're not an expert on TypeScript-JavaScript interoperation; unfortunately neither am I 😅

1. I think that rather than touching the github workflows, it would be better to make the TypeScript testing one of the actions that build-and-test does, just like test:node currently is. That's less disruption to the current structure, and eliminates changes to one file.

Makes sense! Done.

2. I agree that rather than a formal test harness for TypeScript, since this project is not so much about TypeScript, it's fine to do ad-hoc testing just by running types/index.ts. In that spirit, the current types/index.ts has lots of comments indicating what the results of the operations should be, but there's nothing that verifies that what the comments say actually turns out to be the case. Is there an easy way to include an assertion scheme into the TypeScript file so that those comments can be converted to bits of code that will actually fail if the results are not as expected? (I am not saying parse the comments, just to have a facility so that any of the comments could be converted into a check, and do the conversion on at least some of them as a start, with some kind of intelligible output in case of failure.) Is it possible just to import assert into index.ts (even though assert is a JavaScript library) and change comments into assertions?

Also a good idea! Done.

3. And if running types/index.ts comprises the TypeScript testing that we are doing, then it seems to me important that it encompasses the type inferencing that we want to enable/ensure. To do that in an comprehensive way is I am sure beyond the scope of this PR (certainly we do not want to make this too big a chore for you!) but since the original point of your submission was to ensure that TypeScript knows that the outcome of adding two Matrix objects is a Matrix, it should be the case at least that types/index.ts will only compile/run without error if that inference can be made. Is that the case for the current types/index.ts? If not, I'd recommend that you add a section devoted to type inference testing, commented as such, which does test that inference (in the usual spirit of adding tests for issues that you fix). (You may want to add a general comment at the top of index.ts that the file serves a dual purpose as TypeScript examples and testing.) Then future PRs can build on that kernel of type inference testing.

Yep, sounds good! Done.

4. Finally, I think it is fine to include the "add/subtract" improvements that you want in this PR -- they are minor, and they provide a nice example for extending types/index.ts to encompass type inferencing. (On that topic, did you try just

add<T extends MathType>(x: T, y: T): T;

that you mention above, in place of the current typing and your addition?

Fair enough! And no, I hadn't tried that before, but I have now changed this PR to do that and it seems to work well.

It seems to me that it might work for adding a number and a Complex, since doesn't number | Complex extend MathType, so that there is an add(x: number|Complex, y: number|Complex): number|Complex instance of this template, so that TypeScript would allow the addition and know that the result is of type number|Complex (which is technically correct)? I am not really sure whether TypeScript can do this, but it certainly seems worth a try.)

Unfortunately that doesn't quite give what we'd like:

math.add(42, math.complex(4, 2)) // error: Argument of type 'Complex' is not assignable to parameter of type '42'.
math.add<number | math.Complex>(42, math.complex(4, 2)) // works! but kind of verbose

Since I'm guessing people might not want to have to explicitly write these generic parameters in this case, I also left in the plain MathType signatures that were there before.

Thank you so much for working together with us on this!

Thanks for being so welcoming!

[gwhitney]

Nice! I think we are getting close on this. As far as running ts-node without modifying package.json goes, I can tell you something that seems to work at least partially:

• Copy package.json to types/package.json
• Edit that new copy of package.json to not have the type: 'module' line
• execute ts-node --cwd-mode --cwd types index.ts

Maybe you can check out the ramifications of this and/or see if there's a way to simplify it? My main concern with the above exactly as described is that it would be really problematic and non-DRY if we had to have two versions of package.json in two different directories that had to be kept in sync except for that one line being present or not. So I wouldn't really find that to be an acceptable final solution. But maybe the package.json in the subdirectory can be very much of a stub that wouldn't have to change at all typically and doesn't reiterate much if any of the info in the "real" one. Or something along those lines. Or maybe there's a way of including the main one and just overriding that one setting. Etc. If you can work something along those lines out, it's likely better than attempting to change a major global parameter, especially with the odd behavior you were seeing. Of course, Jos may have other ideas.

Since I'm guessing people might not want to have to explicitly write these generic parameters in this case, I also left in the plain MathType signatures that were there before.

It looks kind of weird because it seems as though add(x: MathType, y: MathType): MathType; is an instance of the template that TypeScript should therefore already know about, but hey, can't argue with the actual behavior. I am content with the new typing you've arrived at; it's certainly more functional than the previous typing, even if it doesn't yet let typescript know that add of a number and a Complex is a Complex.

So I think as soon as we get the last kinks about running ts-node and the type: 'module' setting worked out, this will be good to go and a nice start to beefing up the TypeScript stuff in mathjs.

[gwhitney]

It appears that another option for running types/index.ts is given by a later answer to the same SO question: https://stackoverflow.com/a/69055912

Specifically, in our case that would consist of
A) leaving "type": "module" in package.json
B) Changing the value of "module" in types/tsconfig.json to "ESNEXT"
C) Changing the definition of script "test:types" in package.json to "cd types && node --loader ts-node/esm ./index.ts"

If that also seems to work for you, then maybe we should try going with that? Having read a bit more about its meaning, I don't like the look of removing "type": "module" from package.json. Let me know your thoughts or if you have another alternative to suggest. Sorry this bit of interoperation between TypeScript and JavaScript is kind of mysterious.

[josdejong]

Thanks @samestep for working on the type definitions (and @gwhitney for reviewing). In the long run it would be great to have mathjs implemented in TypeScript (I think), but that will be a huge undertaking. Having the type definitions unit tested will be of great help to keep it all in sync.

In general, mathjs is very flexible with types, I think it will be hard to capture the current behavior for 100% in strict types. So I guess for the time being we'll have to be happy with like 95% accurate types and accept some limitations.

About the "type": "module" change: "type": "module" is indeed important and necessary. For packages consuming mathjs it let's them know that the package is not commonjs but ES module. And also just inside the project itself it let's Node.js know that the code is written as ES module and not Commonjs. I've had much troubles with TypeScript too regarding this missing file extensions (till the point of dropping TypeScript from projects because of it). You can read more about that issue here: microsoft/TypeScript#40878 (comment). I would love to see a working solution where TS works to gether nicely with modern ES modules. If we need to make changes in "type": "module", we will need to verify very carefully whether it doesn't break consuming the library from all kinds of frameworks and setups (wepback, rollup, typescript, react-create-app, etc, etc). It can very well be that there are different approaches to this.

[samestep]

Sorry for dropping the ball on this!

@gwhitney I modified this PR to use your last suggestion and it seems to work. I had already tried this previously, but didn't use it initially because it prints this warning:

> mathjs@10.4.0 test:types
> cd types && node --loader ts-node/esm ./index.ts

(node:4203) ExperimentalWarning: --experimental-loader is an experimental feature. This feature could change at any time
(Use `node --trace-warnings ...` to show where the warning was created)

This is better than removing "type": "module" though.

@josdejong Are there any further changes you'd like me to make to this PR?

[gwhitney]

All the tests pass and I think this is an important step forward. I've now also looked over the code changes in detail and all looks reasonable to me, except I am not clear what was going on with the reviver() change. Once that question is answered, Now that that question is answered, my personal vote would be to merge this; of course for something like this, Jos will make the final call.

[josdejong]

Glad to hear you managed to solve it without changing "type": "module" @samestep . Looks good, thanks for all your work!

[josdejong]

Published in v10.4.1