Skip to content

wip: porting to typescript #23

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 4 commits into from
Closed

wip: porting to typescript #23

wants to merge 4 commits into from

Conversation

endel
Copy link

@endel endel commented Sep 28, 2019

Hey @gordonnl,

Thanks a lot for your efforts on this library, it looks really clean. I wonder if you have strong opinions against using TypeScript?

I've started porting it to TypeScript today, as the codebase is quite large I'm going to open this PR as a draft for now, if you like it I can continue the migration. (there are still 498 compilation errors due to lack of type annotations. )

TypeScript helps a lot avoiding silly mistakes (e.g. calling nonexistent methods, or passing wrong type of arguments), provides code completion for end-users, and more good stuff.

Let me know what you think! Cheers

@gordonnl
Copy link
Contributor

Hey @endel,
Thanks a lot for your work! This looks like a solid transpiling.

I do agree that strong typing would be great for javascript, however I'm not currently a typescript user. If TypeScript were supported by browsers natively then I would jump onboard, however I really like not requiring a build step during development, and on the same note not forcing others to require one also.

@endel endel marked this pull request as ready for review September 28, 2019 23:53
@endel
Copy link
Author

endel commented Sep 28, 2019

Thanks for the feedback @gordonnl, I understand additional compilation steps can be annoying sometimes. The fewer barriers the best.

I've "completed" the TypeScript port. Here are a few compilation errors that may actually result in runtime errors:

src/core/Renderer.ts:390:51 - error TS2339: Property 'depth' does not exist on type 'RenderTarget'.

390             if (this.depth && (!target || !target.depth)) {
                                                      ~~~~~

src/extras/GPGPU.ts:63:13 - error TS2322: Type 'Float32Array' is not assignable to type 'HTMLCanvasElement | HTMLImageElement'.
  Type 'Float32Array' is not assignable to type 'HTMLImageElement'.

63             image: floatArray,
               ~~~~~

  src/core/Texture.ts:32:5
    32     image?: HTMLCanvasElement | HTMLImageElement;
           ~~~~~
    The expected type comes from property 'image' which is declared here on type 'Partial<TextureOptions>'

src/extras/Orbit.ts:128:45 - error TS2339: Property 'fov' does not exist on type 'Transform'.

128         targetDistance *= Math.tan(((object.fov || 45) / 2) * Math.PI / 180.0);
                                                ~~~

src/extras/Skin.ts:90:13 - error TS2322: Type 'Float32Array' is not assignable to type 'HTMLCanvasElement | HTMLImageElement'.
  Type 'Float32Array' is missing the following properties from type 'HTMLImageElement': align, alt, border, complete, and 262 more.

90             image: this.boneMatrices,
               ~~~~~

  src/core/Texture.ts:32:5
    32     image?: HTMLCanvasElement | HTMLImageElement;
           ~~~~~
    The expected type comes from property 'image' which is declared here on type 'Partial<TextureOptions>'


Found 4 errors.

Here are some notes regarding the typings and changes I've made (which can be improved a lot! I've just made it compile atm)

Renderer.ts

  • RendererState: turn into an interface. Fill all possible options.
  • RendererParams: turn into an interface. Fill all possible options.
  • RendererExtensions: turn into an interface. Fill all possible options.

RenderTarget.ts / Texture.ts:

Inherit constructor options from the same interface instead of duplicating all the values.

Orbit.ts

  • Refactoring: changed element = document to element = document.body

Vec3Func.ts / QuatFunc.ts

I had to use number[] | Vec3 in some places and it feels wrong. Maybe we should have a type definition for all vector-like structures like:

type Vec3Like = number[] | Vec3 | Float32Array

@endel
Copy link
Author

endel commented Sep 28, 2019

I've also changed the examples to continue to work, using the compiled JavaScript files generated by tsc.

@3mcd 3mcd mentioned this pull request Sep 29, 2019
@gordonnl
Copy link
Contributor

Thanks for this mate, will go through it today

@gordonnl
Copy link
Contributor

gordonnl commented Oct 2, 2019

Ok I've had some time, thanks again for this - it's a great learning reference for me.

TL;DR I think it's too early, once OGL hits beta I'd be willing. Or, if there was browser support it'd be no issue.

For the errors, the first one was a helpful find, thanks - I've added depth as a public parameter. The others are due to a non-complete type declaration. The image param with gl.texImage2D is quite versatile, here is the full list from MDN:

  • ArrayBufferView (Uint8Array, Uint16Array, Uint32Array, Float32Array)
  • ImageData
  • HTMLImageElement
  • HTMLCanvasElement
  • HTMLVideoElement
  • ImageBitmap

For the Math Function classes, they can work with number arrays or typed arrays, they don't require any of the extended class functionality, so I'd remove the cyclic dependencies as you're not forced to pass in specific Math instances.

So, I think that there are pros and cons, one of the main goals for this library was to keep it as simple as possible, enhancing its use as a WebGL learning resource, and as a beginner-friendly code-base. I obviously see the benefit that types bring to an IDE, however the required build step is a process I'm very much looking to avoid.

The library still has some missing holes, largely multi render targets, depth textures, cube-maps, shadow maps, GLTF, along with others. As I don't use TypeScript, this would make it more difficult for the author, slowing progress and adding complication. Once the library is in a happy state and I don't have glaring changes to implement, I'm willing to revisit.

@endel
Copy link
Author

endel commented Oct 12, 2019

Thanks for the clarification, agreed, I totally understand your concerns :)
Wish you the best of luck with this project! Cheers!

@endel endel closed this Oct 12, 2019
@endel endel deleted the typescript branch October 12, 2019 14:30
@endel endel restored the typescript branch October 12, 2019 14:30
@munrocket
Copy link

Why this PR was declined? This is a key point where you library @gordonnl can be better than Three.js! Types in math/webgl libraries not so uncomfortable as in frontend. Also you will never create errors like this anymore.

@eXponenta
Copy link
Contributor

eXponenta commented Nov 2, 2019

Or this #29

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants