search

BertrandBev / eigen-js

⚡ Eigen-js is a port of the Eigen C++ linear algebra library
https://bertrandbev.github.io/eigen-js/#/
92 stars 15 forks source link

Add type declarations #37

Closed samestep closed 2 years ago

samestep commented 2 years ago

Resolves #33.

A few notes (in no particular order):

  1. This PR only adds a type declaration file; it does not add any checks for those type declarations. While developing this PR I renamed src/eigenTest.mjs to src/eigenTest.ts and observed type errors as a smoke check, but I couldn't figure out how to run that file (e.g. via ts-node) so I undid that change.
  2. I'm not sure exactly how the documentation site is generated (I see docs/doc.json but I don't know how to generate a website from that), but it seems to be fairly out of sync with the code. I'm guessing this is just because it only uses src/classes/, which seem to be manually written? In any case, I decided to completely ignore the documentation website while writing these type declarations.
  3. In a similar vein to the above two notes, this PR does not provide any way to autogenerate src/eigen.d.ts or otherwise keep it in sync with the rest of the code.
  4. I couldn't tell if anything was meant to be a hidden implementation detail (e.g. the static fields objects and whitelist on eig.GC, or the methods vGet and vSet on eig.Matrix), so I just exposed everything.
  5. I saw #8 but wasn't sure how to reflect that in these type declarations, so they assume that OSQP is always present.
  6. Although src/cpp/embind.cc defines a ComputationInfo enum, eigen.mjs does not attach that enum to the exported eig object, so currently the info fields on EigenSolverResult and CareSolverResult seem to be unusable. This is reflected by the fact that src/eigen.d.ts equates ComputationInfo with unknown.
  7. I'm not quite sure what the distribution process is, since the build instructions in README.md mention a folder called build/, but package.json mentions a folder called dist/. I just left eigen.d.ts in src/ and told package.json to use that for the types, since that seemed simplest if src/eigen.d.ts is handwritten.

Let me know if you'd like me to make any changes before merging.

samestep commented 2 years ago

@BertrandBev Thanks so much for merging this! Would you be able to release a new version of the package to npm?

BertrandBev commented 2 years ago

@samestep sorry for the late reply. I just published v2.1.0 to NPM!

samestep commented 2 years ago

@BertrandBev Amazing, thank you! Unfortunately it looks like Eigen.js v0.2.1 on npm is missing dist/index.js (so it has the types but not the code); would you be able to publish a v0.2.2 which includes that? Sorry for the hassle!