TS: Change Geometric Object property types to generics

[pailhead]

This seems to resolve the issue, and when using the same syntax, you can get the expected type.

const myMesh = new Mesh( new Geometry(), new MeshBasicMaterial())

myMesh.material.map //error, map is not a property of Material | Material[]

With this, myMesh.material will be of type MeshBasicMaterial.

How to format this, and should there be an alias given to Foo | Bar or Foo | Foo[]? (no, because some classes use different signatures eg InstancedMesh)

Fixes #19072.

[donmccurdy]

I think that using generics would be an improvements to the type definitions, and that this is a good direction, but unfortunately I'm not able to approve this change. Doing so will require testing, preferably from someone who already has a nontrivial TypeScript project using three.js, and I don't feel that I can do that right now.

A few things should be checked:

• Do the lint tests pass? npm run test-lint and npm run test-lint-examples do some sanity checks on the type definitions, and should be run for any changes to the definitions.
• Do other uses of Mesh now have to be parameterized? E.g. Geometry.mergeMesh( mesh: Mesh ) might need to be parameterized as Geometry.mergeMesh( mesh: Mesh<Geometry, Material> ), if the TS compiler requires it.
• Is this a breaking change? I think so, in the sense that TS code that compiled in r116 will no longer compile in r117. That's OK, if TS users feel it's an improvement, and I believe that it is. But we should call this out in the changelog.

Perhaps @samuelint, @jedahan, or @fms-cat can comment? It looks like this change would also be covered by #18720, although there are other changes.

[0b5vr]

Yes, I think this PR is a duplicate of #18720 that has several resourceful discussions. It seems @jedahan -san is so busy though...

Do other uses of Mesh now have to be parameterized? E.g. Geometry.mergeMesh( mesh: Mesh ) might need to be parameterized as Geometry.mergeMesh( mesh: Mesh<Geometry, Material> ), if the TS compiler requires it.

That is a discussion I've never seen in #18720 and I think that is important.

[donmccurdy]

It is possible to provide default values for generics in more recent versions of TypeScript (microsoft/TypeScript#13487), see Generic Parameter Defaults in TypeScript. I have never seen or used that before though. Otherwise they are required.

[pailhead]

Yeah i think that's another question, is there some reasonable version of TS that's considered current, but not bleeding edge?

[pailhead]

What is the effect of assigning something to the generic, i'm not yet familiar with this syntax? But to confirm this seems like a duplicate.

[pailhead]

mergeMesh() seems to be doing fine, but this cast below is complaining.

Edit, which the = Material | Material[] seems to solve :D I guess these are the default generics? TS 3.8.2

[pailhead]

The second point seems like it's resolved with this, if it fits the TS version.

Is this a breaking change?

Not sure how to investigate this. The syntax seems to remain, ie there's no need to do Mesh<Foo,Bar>( new Foo(), new Bar()) just pass the arguments.

Lint testing the examples spits out a bunch of warnings, but none seem to be related to this. Lint alone reported nothing, so i assume no warnings or errors?

[pailhead]

Is it actually needed to use <> if the super class already has it?

[donmccurdy]

I think it's necessary, yes, unless a parent class has overridden the generic types with discrete types. For example:

class A<T> {}

// Must parameterize T.
class B extends A<String> {};

// Parameterization not needed.
class C extends B {}

[donmccurdy]

Although, maybe not with defaults? I haven't tested those before.

[pailhead]

I've added the default generics, which seems to have made some issues go away and touched the rest of the geometric Object3Ds.

[0b5vr]

Also could you refer this comment to add default types for these constructors?

#18720 (comment)

Edit: Oh, you already were seeing the comment. Thanks

[0b5vr]

Yeah i think that's another question, is there some reasonable version of TS that's considered current, but not bleeding edge?

This is an interesting topic.

Since DefinitelyTyped says they are not going to support TS versions that is over 2 year old, I think we can say versions that is lower than 3.0 are out of scope.
https://github.com/DefinitelyTyped/DefinitelyTyped

[0b5vr]

also mark this PR to close #19072 maybe?

[Mugen87]

also mark this PR to close #19072 maybe?

Done.

[pailhead]

Renamed to TGeometry and TMaterial. @fms-cat i didn't realize, the TS issues referenced are from 4 years ago, looks like we're using standard features by now here?

[0b5vr]

Yes, I think we are very ok to use default type parameters here. So many modules in DefinitelyTyped utilizes it, since it's a feature that provides us an ability to gain both compatibility (with JS) and extensibility at once.

[pailhead]

All three of the issues that @donmccurdy i think pass now, i'm not sure what else to look at, but i'd like to test it some more. I wanted to ask about the testing approach, is it possible to merge this in dev and expect issues to be reported, or would it be better to increase the confidence that the PR would break as few things as possible? I'd also like to squash this, not sure if the author should do it, or happens automatically?

[mrdoob]

@donmccurdy looks good?

[pailhead]

If merged to dev i can be referenced for any issues that arise.

[WestLangley]

I'd also like to squash this, not sure if the author should do it, or happens automatically?

@pailhead You will need to do your own squashing.

[pailhead]

Actually, this is no longer just for Mesh but several classes. Does this title make sense?

[donmccurdy]

This change looks OK to me, but I don't really have a way to test it... I'm hoping someone using three.js in an existing TS project can test it (e.g. checkout the PR, run npm link && npm run dev, then run npm link three in your project) to confirm that it compiles either without errors, or with errors that seem acceptable.

[elalish]

@donmccurdy Ah, I did try that, but I did it with const mesh = new Mesh(geometry). I was expecting to get the default MeshBasicMaterial, but I did not.

[donmccurdy]

Sounds good — Yeah, the actual/runtime default is a discrete type (MeshBasicMaterial), but the type definition's default is a superset of all possible values, which I think is correct.

[elalish]

@donmccurdy Hmm, I changed it to const mesh = new Mesh(geometry, new MeshBasicMaterial()); and I still get exactly the same errors about properties not existing on Material | Material[].

[pailhead]

Hmm, could this be a typescript version issue or something? I’m not getting errors on my end.

[pailhead]

Also feel free to test the geometry as well. It should pick up the properties between g and bufferg.

[elalish]

@pailhead I'm on 3.7.5 of typescript, which in fact does look a bit out of date. What version are you testing?

[pailhead]

A bit newer 3.8 something but it doesn’t seem like it should be affected, it’s recent enough? Did you link properly?

[pailhead]

@donmccurdy I didn’t understand your comment about the mesh coming from a loader. The expectation is that even a loader should type the Mesh properly when passing the material/geometry type. Do you expect the same? Ie no syntax should be changed in a typescripted loader, but we should see the benefit?

[donmccurdy]

@elalish if you're in VSCode and hover over the mesh local variable, can you see what types have been inferred? Does it show anything about the generic parameterization at all? For that matter it might be worth making a local change to the type definitions (e.g. set TMaterial extends HTMLElement) just to force an error and ensure it's npm link'd properly.

@pailhead the type of material coming from the loader is not available at compile time; TypeScript doesn't know what you're going to load. For something like OBJLoader maybe you could override the loader's type definitions, knowing that MTLLoader can only create MeshPhongMaterial, but TS won't know that.

[pailhead]

First off, to not freak out @mrdoob this is unrelated to the change, it's just conversation, this seems like it's good to go pending tests.

@donmccurdy
So i'm looking at this in OBJLoader and trying to wrap my mind around it:

Is the first stumbling point that this is sort of beyond interfaces, ie. if this code were in typescript, it would actually type correctly, based on whatever instance is in this createdMaterials ?

[donmccurdy]

I'm not suggesting we change any loaders. Technically you could write a loader in TS that might provide better type information to the user, but it's complicated in several ways that I'd rather not get into.

[pailhead]

But can you at least help me clarify this without getting too much into it :)

My understanding is that since we only expose interfaces, without implementation in TS, that this default generics, or generics in general cannot reach as deep. Ie. there is no way for me to define an interface to the loader (except maybe by explicitly doing <Phong>ObjLoader.load('obj_with_phong.obj)? Either way, what drives my TS where i declare Mesh is not available to figure out the types if new Mesh() is not in TS?

[donmccurdy]

To have type information from a loader written in JS, you would need to update its type definitions to something like:

type ObjLoaderResult = Mesh<BufferGeometry, MeshPhongMaterial>;
load(url: string, onLoad: (result: ObjLoaderResult): void, ...): void

That becomes unsolvable in a method that returns Group or Scene, without decorating all sorts of new things with type information.

The generic types in this PR will help users who are creating Mesh and Material instances in their own TypeScript code. It will not affect the results of methods that return Group or Scene instances, or whose return types are not known at compile time, even if those methods are written in TypeScript.

[pailhead]

Right, there is no way to cast it from just some arbitrary data thats loaded. Works with OBJ since it only returns one type, so the whole loader can be cast. Thanks for clarifying.

[pailhead]

@elalish any luck?

[elalish]

@pailhead No, but I'm guessing it's because I'm using the properties in a different function than where the mesh is defined (I made it a member variable). I don't think TS is smart enough to push those types across a boundary like that. That is part of why this change seems pretty narrow to me. To really get materials to be more easily usable, we'd need to get rid of Material[] as an option entirely, which I assume is not feasible for some reason. Anyway, this change doesn't break me, so I'm fine with it; it just won't help my TS project at all.

[pailhead]

Any chance you could post some code to illustrate your situation?

@mrdoob I think at best this solves the issue people have posted about, at worst it won’t do anything. Any chance it could be merged?

[donmccurdy]

I agree, it looks okay to merge this.

[Mugen87]

It seems this PR closes #17564, right? Enhancing Fire.d.ts similar to #17564 could make sense, too.

[pailhead]

Looks like it. Though Fire.d.ts is in examples, maybe it would make sense to do this type of change, for examples, in a different PR?

Does this PR cover all the geometry/material objects in the core?

[Mugen87]

Though Fire.d.ts is in examples, maybe it would make sense to do this type of change, for examples, in a different PR?

If you can change the core declaration files without breaking the one in the examples file I'm okay with that.

Does this PR cover all the geometry/material objects in the core?

I would say yes.

[mrdoob]

Thanks!

[0b5vr]

ohhhh it got merged, thanks!