Add types using JSDoc to the graphlib file#60
Conversation
Vitest adds these automatically, so it didn't cause any issues when running the tests, but it did cause issues with TypeScript.
This lets TypeScript check that the functions we call on this type are valid.
Update the `tsconfig.json` file to also type-check the tests, checking for any potential mistakes we make in TypeScript. The new `tsconfig.build.json` file is then used for actually building the types.
I've manually gone through the JSDoc references within the `src/graphlib/graph.js` file and added JSDoc comments when TypeScript couldn't automatically find detect the types. I've tried to keep the types as backwards compatible as possible. There's also an MIT-licensed [`@types/graphlib`][1] package that we can copy code from, but I didn't do that since I noticed issues with it that were causing our unit tests to fail type-checking (e.g. not allowing `number`s as params for lots of functions, even though the test code uses it a lot). [1]: http://npmjs.com/package/@types/graphlib
|
Do you want me to release a new version for it? Or do you want to test more in mermaid first? |
|
I've tested it with Mermaid and it looks okay to me! It even caught some bugs in Mermaid, there was some code calling graphlib with the args in the wrong order, see: Up to you if you want to make a new release or not 🤷. This PR is just a types change, so it shouldn't affect any actual code, but it could be worth releasing this with #51. It's a long weekend in Japan, so if I have free time, I might make another PR trying to add types to the rest of the the |
Update the Graph documentation with a mixture of: 1. API documentation from the graphlib wiki: https://github.com/dagrejs/graphlib/wiki/API-Reference 2. Documentation from https://github.com/dagrejs/graphlib/blob/64b31ef3df0270c9bda85b45cb338481ce25618d/lib/graph.js Both of these are from the graphlib MIT license, which we're already complying with.
|
I've just pushed another change. I've copied over the official documentation from graphlib with a mixture of:
Both of these are from the graphlib MIT license, which we're already complying with. |
2 participants
Similar to #45, I've gone through the
src/graphlib/graph.jsfile and added proper types to the functions whenever TypeScript couldn't automatically figure out the types.I've also added type-checking of the
*.test.jsfiles to ensure that at least the types match what the unit tests expect!Stuff I didn't do and why
There's also an MIT-licensed
@types/graphlibpackage that we can copy code from, but I didn't do that since I noticed issues with it that were causing our unit tests to fail type-checking (e.g. not allowingnumberparams for lots of functions, even though the test code uses it a lot). We could copy it over later if we want to have better descriptions for the graphlib functions.I also tried to apply 8740b83 from #45 and unfortunately it seemed to cause errors in other parts of the code, since they're still not typed properly. But all the graphlib types looked correct to me.