Contribute TypeScript type files for most Three.JS JavaScript files

[LuWang9018]

In reference to the previous conversation involving @bhouston and @mrdoob, I've created Typescript type files (*.d.ts) for most JavaScript files in the Three.JS project.

See here for the motivator: #11552 (comment)

[donmccurdy]

Thanks @LuWangThreekit! From #11552 (comment), is it possible to automate testing this somehow? Against the source files themselves, or our existing examples and tests files, perhaps?

[LuWang9018]

Thanks @LuWangThreekit! From #11552 (comment), is it possible to automate testing this somehow? Against the source files themselves, or our existing examples and tests files, perhaps?

Hi @donmccurdy, I have never done automate testing before. I will do some search to see how I can achieve this. Is there any chance that you can give me some directions?

[donmccurdy]

To be honest I don't know for sure whether it's possible, but it would be helpful to find out before we take ownership of the TS typings. Based on this documentation it's possible to compile JS while checking it against typings and JSDoc. The three.js codebase does not use JSDoc, so practically that may or may not work... ideally we'd be able to do something like this...

tsc --allowJs --checkJs src/
tsc --allowJs --checkJs examples/js/

...and a failed compile would tell us that the typings have gotten out of sync with the core library.

[LuWang9018]

To be honest I don't know for sure whether it's possible, but it would be helpful to find out before we take ownership of the TS typings. Based on this documentation it's possible to compile JS while checking it against typings and JSDoc. The three.js codebase does not use JSDoc, so practically that may or may not work... ideally we'd be able to do something like this...

tsc --allowJs --checkJs src/
tsc --allowJs --checkJs examples/js/

...and a failed compile would tell us that the typings have gotten out of sync with the core library.

Thanks! I will check to see if this will work.

[bhouston]

BTW to be clear, this is based on the existing typings here: https://github.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/three Lu wrote an automated method to split it apart.

[donmccurdy]

If three.js is not using TS or planning to move to TS, and if we have no way to verify that these types remain correct with library changes, I'm not sure we should be taking over their maintenance. The existing contributors to the DefinitelyTyped are presumably using TypeScript themselves and can check the types against their own projects, which I think means they will do a better job than we are likely to do. I worry that by just moving the types here, without an owner or a plan to maintain them, we're essentially saying, "don't worry, we've got this" to the current maintainers without a good chance of success.

That said, I appreciate the effort here to improve workflows for TS users, and would welcome ideas on how to make that robust and sustainable. 🙂

[Methuselah96]

I agree with @donmccurdy. I think the @types/three package is a much safer bet on keeping the type definitions up-to-date. It is very easy for anyone to make changes to the type definitions there and until/if three.js starts using TypeScript, these type definition files will just be extra baggage, especially for those who don't use TypeScript. It also puts an extra burden on the maintainers that right now seems to be easily handled by the DefinitelyTyped community. What do we gain by moving the type definitions to this repo?

[sethk]

As a consumer of three.js that uses @types/three, it's a huge PITA to have to make local changes to the DefinitelyTyped repo (it's huge and I use a git sparse checkout), possibly update the tests, and then run npm link to get it into my project. The types in the API contracts are now in three places: sprinkled throughout the source as runtime assertions like the one below, in the documentation (the canonical source, except where it's inconsistent), and in DefinitelyTyped.

At least if the type definitions were in the same repo, one could change them side-by-side, and incrementally move towards automated tests and generated method signatures in the documentation, instead of a duplication of effort that doesn't really seem like it should be done by humans.

With regard to adding an extra burden to the maintainers of three.js, that cost is already included in the expectation that the documentation will be complete, correct, and up to date. It's just that currently the validation of such is done by humans filing bug reports, rather than by a type checker. In practice, this burden is shifted out to the users, and to TypeScript users (judging from the submissions to DefinitelyTyped there seem to be many) who have to maintain the definitions in a separate repo. These extra costs do not scale very well, as they are reactive rather than part of the CI process, and it's often not clear whether your program is wrong, the typings are wrong, or three.js has a bug.

I also feel certain that if three.js moved towards augmenting their API with typings, it would expose trivial bugs in both the implementation and documentation.

		if ( ! ( geometry && geometry.isGeometry ) ) {

			console.error( 'THREE.Geometry.merge(): geometry not an instance of THREE.Geometry.', geometry );
			return;

		}

[Methuselah96]

@sethk You would still have to npm link three.js if you needed to fix TypeScript definitions, right? Is it just an issue because DefinitelyTyped is so large?

That would be great if there could be some sort of validation of the TypeScript definitions or if the TypeScript definitions were used to generate the documentation in order to reduce redundancy. I feel like the TypeScript definitions would be redundant until that tool is created.

My main concern is how long it takes for three.js PRs to get merged and published. With DefinitelyTyped, I can make a change, make a PR, and have it approved, merged, and published so that I can use it, usually within 2-3 days.

It would be great if the three.js contributors/maintainers are all willing to keep the TypeScript definitions up-to-date as they make changes, but it just adds another level of barrier to have to know how TypeScript works. At least in the earlier days of TypeScript, there weren't enough people who felt confident in their TypeScript knowledge to make this work on some repos.

I would be much more excited if we actually started converting some of three.js to TypeScript to reduce redundancy even more. Or if we started by adding JSDoc and then added the TypeScript definition files then we could avoid redundancy there.

[sethk]

@sethk You would still have to npm link three.js if you needed to fix TypeScript definitions, right? Is it just an issue because DefinitelyTyped is so large?

My main issue is the denormalization of the Three.JS API contract into three separate places. All the yak shaving I had to go through in order to get my project to compile is just a symptom of that.

Using npm link is probably something people who have been working with NPM for a while would think to do with the DefinitelyTyped modules, but as a new user to both NPM and TypeScript, it took me quite a long time to figure out that shortcut. First I tried augmenting the types from inside my project using some module and namespace declarations, which didn't work because of some complex issue with how the top-level THREE module is exported. Then I tried referencing a git cloned copy of the three/index.d.ts file from my source, which also didn't work because no matter how you configure the TypeScript compiler, it treats @types/three as a special kind of overlay to three/, otherwise it complains about duplicate exports.

Anyway, all of that was a pretty high bar for just using Three.JS from a TypeScript app, and if I hadn't already felt very committed to using TypeScript for the rest of the app, I probably would have abandoned it.

That would be great if there could be some sort of validation of the TypeScript definitions or if the TypeScript definitions were used to generate the documentation in order to reduce redundancy. I feel like the TypeScript definitions would be redundant until that tool is created.

They are definitely redundant, but at this point they're redundant and also not colocated.

My main concern is how long it takes for three.js PRs to get merged and published. With DefinitelyTyped, I can make a change, make a PR, and have it approved, merged, and published so that I can use it, usually within 2-3 days.

Yeah, that is a valid consideration. However, it seems like PRs to Three.JS that introduce new APIs or change old ones must also include a step to update the documentation, which would be a natural place to update the TypeScript declarations as well. Separate PRs that update only the typings should be pushed through fairly quickly, along the lines of a low-risk bugfix.

It would be great if the three.js contributors/maintainers are all willing to keep the TypeScript definitions up-to-date as they make changes, but it just adds another level of barrier to have to know how TypeScript works. At least in the earlier days of TypeScript, there weren't enough people who felt confident in their TypeScript knowledge to make this work on some repos.

For the less polymorphic APIs, this doesn't seem like it should be that big of a hurdle. If a person making an API-level change understands the full implications of the change they are making, they should be able express this using normative type declarations, rather than the informal type signatures used in the documentation.

I would be much more excited if we actually started converting some of three.js to TypeScript to reduce redundancy even more. Or if we started by adding JSDoc and then added the TypeScript definition files then we could avoid redundancy there.

I agree, but a conversion seems like it would require a lot of buy in. A combination of JSDoc and TypeScript definition files to replace the existing documentation seems like a good stepping stone. What if the documentation could be generated from JSDoc comments in the definition files?

My whole point is: please don't abandon this unification effort just because the intermediary steps seem redundant!

[Methuselah96]

@sethk You have persuaded me that this is probably worth it. I played with TypeDoc a little bit to see how hard it is to generate documentation from TypeScript definition files. From what I can tell, it would take quite a bit of work (custom tooling) to generate documentation from the TypeScript definitions that is the same as the current documentation. One thing that comes to mind that might be a problem are the Chinese translations of the documentation. I'm not sure if there's any way to provide both English and Chinese translations of the documentation and it doesn't seem like both of those should go in either JSDoc or TSDoc comments.

[LuWang9018]

@donmccurdy Sorry for the delay. Can I assume that all types are correct if all the existing examples can run without raising any error in the console? If so, I have written a script to test all the examples. 98% of the examples passed. Those which fail the test are because of @types/three also does not have the declaration for them.

[LuWang9018]

@donmccurdy Thanks for the idea of checking Three.Js against its TypeScript definitions. It actually works really well. Here is the results I got:

https://gist.github.com/LuWangThreekit/962d60e5b2d44aa898e4174c77bdbdb4

I used this command line:

tsc --allowJs --checkJs src/Three.js --outDir ./tmp

Thus there was actually a bunch of errors in the DefinitelyTyped Three.js project. But it is trivial now to identify them and fix them. I do not think any of those are in Three.Js proper though, but maybe if I can fix these it will reveal more.

The issue with running tsc against the examples files is that they do not include their dependences as they use very old style JavaScript. If we could modernize their code, then we could do the same approach.

I think that once I get these errors fixed, we could just add a simple PR checker that continues to keep these updated for the main part of Three.JS. I wouldn't be surprised if it catches obvious bugs in Three.JS going forward.

[donmccurdy]

That's great to hear, thanks @LuWangThreekit! If the type declaration tests are running against our source but not the examples folder, that's totally fine with me. In the future it would be great to (1) clean up existing issues in the typings, and (2) get the test running against example code as well, but I don't think either is necessary in this PR.

With that test functional, I have no objection to bringing the typings into this repository. 👍

[LuWang9018]

@donmccurdy I just fixed all the errors which reported by tsc --allowJs --checkJs src/Three.js --outDir ./tmp. I built and tested all the examples, they all can be run without errors.

[Methuselah96]

@LuWangThreekit Good work! Can we add that command to the CI build so that it will test it whenever a PR is made?

[donmccurdy]

Can we add that command to the CI build so that it will test it whenever a PR is made?

For my $0.02 we can leave that for a future PR; for a decision about whether to merge this PR I think it's enough to know that it can work in CI. :)

[LuWang9018]

@Methuselah96 I think we can use "tsc" build the typed threeJS to regular Js style then use CI to build the Js one.

[Methuselah96]

I agree with @donmccurdy. We should just try to get this PR merged as soon as possible and then we can add the build step in a follow-up PR.

[bhouston]

@mrdoob, @WestLangley What are your thoughts on this PR?

[WestLangley]

@bhouston I don't have an opinion on this one.

[mrdoob]

Lets give it a go! 👍

[mrdoob]

Thanks!

[mrdoob]

I think we should move dist/Three.d.ts to build/ and instead of having all the .d.ts in src/ maybe it'll be cleaner to put them in a new folder types/ in root.

@LuWangThreekit would you like to do a PR with these changes?

[LuWang9018]

@mrdoob I will do it now and make new PR. Thank you for marge my PR.

[bhouston]

@mrdoob @donmccurdy @Methuselah96 we've researched having the types in a separate directory, but it appears to be less than optimal. A separate directory for types does not support a folder structure from our research, thus it is 100+ files in a flat directory and the would have a different include structure as well (as it would have to be mapped to the same directory.)

Personally, I think that it is best to leave them side-by-side for manageability because then it fully mirrors the main Three.js files. It would also serve as a reminder to update them. Although I know this isn't optimal if you want to hide the *.d.ts files in their own directory.

But maybe to make TypeScript easier to maintain @LuWangThreekit could contribute a tsconfig.json file (so one just needs to run tsc to compile) and add to the package.json a script that will run the TypeScript test on the JavaScript files -- something like "npm run test-types" or something like that.

[mrdoob]

@bhouston I see... /dist/Three.d.ts is not needed then? Should I delete it?

[mrdoob]

But maybe to make TypeScript easier to maintain @LuWangThreekit could contribute a tsconfig.json file (so one just needs to run tsc to compile) and add to the package.json a script that will run the TypeScript test on the JavaScript files -- something like "npm run test-types" or something like that.

That sounds good.

[Methuselah96]

@LuWangThreekit @bhouston Is there a reason there is both a Three.d.ts and a Three.ts?

[LuWang9018]

@mrdoob @Methuselah96
Since ''src/three.js' is just export other classes. No need to add a three.ts file.
I made a PR to remove 'desc/Three.d.ts' and 'src/Three.ts'.
#15669