jonobr1 / two.js

A renderer agnostic two-dimensional drawing api for the web.
https://two.js.org
MIT License
8.29k stars 454 forks source link

Typescript migration #636

Closed LucaCiucci closed 2 years ago

LucaCiucci commented 2 years ago

In my projects, I find some difficulties in using two.js because of the non exhaustive types definitions. I think it would be much easier for the users to use it if the definitions were complete.

I think that the project would benefit a lot from a typescript conversion, both developemnt and usage would be a little bit easier and no additional time would be required in keeping types.d.ts always up to date.

I think migration would be very smooth:

I could contribute to the conversion, if you want.

jonobr1 commented 2 years ago

Thanks for posting. Sorry about the types.d.ts, it's been a long, slow, and bumpy journey to get them working better for TypeScript users.

Unfortunately, I am not a TypeScript developer and browsers don't read TypeScript, so it's unlikely at this time to port the project to a language that needs to compile. Beyond those surface level issues, I found it difficult to keep the expressiveness of the project using TypeScript. Perhaps, you can enlighten me on those aspects so that the types.d.ts can become complete for TypeScript users.

Two is the default module that associates with a class, but it's also a namespace from which to import other modules. How do you define this in TypeScript? Or export a JS package that features this?

LucaCiucci commented 2 years ago

Thanks for the answer. I see that types.d.ts has evolved a lot in the past months.

Perhaps, you can enlighten me on those aspects so that the types.d.ts can become complete for TypeScript users.

My mistake, when I wrote the issue I was using the wrong .d.ts file from @types/two.js.

Anyway, I think that types.d.ts could be systematically generated using tsc, with "compilerOptions.emitDeclarationOnly": true and "compilerOptions.outFile".

Unfortunately, I am not a TypeScript developer and browsers don't read TypeScript

A possibility would be to use a tsconfig that takes sources from src/ and emits JS (and, optionally *.d.ts' relative to each file) in another folder, for example lib/, or build/src/ to maintain compability.
It would not be necessary to convert the entire project all at once, we could do it gradually and mix ts and js.

I could try to make a proof of concept in a fork, but if you think that it is better to abandon this idea, I think you can close this issue.

jonobr1 commented 2 years ago

I would love to see a proof of concept as a fork.

I've tried to programmatically generate the types.d.ts docs using tsc, but it's pretty bad at reading my code. So, that's why you see the types.d.ts you do today. You can also see the tsconfig settings I currently have here: https://github.com/jonobr1/two.js/blob/dev/tsconfig.json

If it's as easy as you say it is, then I'm all ears. My experience so far has been otherwise.

LucaCiucci commented 2 years ago

I made LucaCiucci/two.js/tree/build_modules with a small part of the sources converted to TS.

Changes to look for:

I created tsconfig-types.d.ts.json to try to generate an acceptable types.d.ts, but I think an easier solution would be to build the files into a separate folder using the new tsconfig.json.

tsconfig-types.d.ts.json

This provides a way of generating types.d.ts using the command npm run types.
I've not verified the correctness of types.d.ts, yet, I will do it when I have time.
At the moment the generated types.d.ts has one problem: class Two is exported from module "two.js/src/two" so it has to be manually modified to declare module "two.js", I suppose.

tsconfig.json

This provides another way of building the library for users:

Users would use it like this:

import { Vector } from 'two.js/lib/vector.js';

:warning: This would break compatibility

In fact, at the moment, the correct way of importing two.js is:

import { Vector } from 'two.js/src/vector.js';

If we want to maintain compatibility, we have three options:

  1. Move the sources in another folder, for example lib_src and emit the compiled JS into src/. This would be simple, but not very beautiful;
  2. emit the JS into build/lib/src (or any other path) and use package.json exports (see katex as an example). At the moment I am not able to use this feature, even the katex package exports don't work for me, maybe I have some wrong configurations or it might be related to this.
  3. release a major version and force users who update to correct their code.

I think the first option would be the best, in this case.


Note that, at the moment, I am not building extras/, but it could be done in the same way.

LucaCiucci commented 2 years ago

In case we adopt the soultion using the new tsconfig.json, the file types.d.ts would not be necessry anymore, I think.

jonobr1 commented 2 years ago

Interesting, nice work. How do you recommend testing the types.d.ts?

LucaCiucci commented 2 years ago

At the moment, I don't know how to test types.d.ts, I will try to find a way but I'm quite busy this month. Maybe someone else has some ideas...
However I think a rough way would be to just create a .ts file that uses all the features and check if it compiles and runs, maybe the package check-dts could be useful, but it would be quite a bit of work.

Anyway, if we drop the types.d.ts in favour of the compiled *.js/*.d.ts pairs, no testing of the .d.ts files would be necessary when the project is completely converted to TS.

jonobr1 commented 2 years ago

Thanks, this is really helpful. Maybe my next action item will be to simply make a big test.ts file so that all the features are accounted for in ways that are expected.

I appreciate the work you've started here, but given that (I believe) we are close to getting the types.d.ts file close albeit manually, I don't think I'll be migrating to TS anytime soon. It took about 2 years to migrate from ES5 to ES6 and there are still many edge cases I'm finding that are broken because of the automated and manual ways that I migrated things. Migrating yet again would only cause more instability for the moment. I'm going to close this issue out, but open up another specific for writing TypeScript tests: https://github.com/jonobr1/two.js/issues/642

Thanks again,