Skip to content
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

Typescript definitions #50

Open
mode777 opened this issue May 30, 2017 · 30 comments
Open

Typescript definitions #50

mode777 opened this issue May 30, 2017 · 30 comments

Comments

@mode777
Copy link

mode777 commented May 30, 2017

I wrote some typescript definitions for twgl, for a personal project. They don't cover the sub-modules yet, but I would be willing to add them.
However, I don't know how (if at all) to contribute them. I could just create a separate repo and publish them to definitely-typed and "@ types/twgl.js" on npm but I feel like these repos/packages shouldn't be owned by me.

Any suggestions?

@greggman
Copy link
Owner

I'd love to take a look. I haven't dived into typescript yet. I'm curious how much verbose it makes things for stuff like twgl.createTextures

@mode777
Copy link
Author

mode777 commented May 31, 2017

The verbosity is managable if you use type inference where you can. Functions like twgl.createTextures will especially benefit from typescript, as you get full intellisense and type checking for all the options:
untitled-1

The definitions themselves sure are verbose as all the types need to be delcared.

Currently I just keep them in my project repository, but I feel they should either be distributed with the main package or in a separate @types/twgl.js package (better choice IMO)

@colorpump
Copy link

Nothing happend? I haven't used yet twgl, but I would, if there were typescript definitions for it.
I'd highly appreciate it.

@mode777
Copy link
Author

mode777 commented Sep 4, 2017

@colorpump : I'm still working on this. Twgl is a huge library, if you use all the sub modules.
Here's a incomplete version, if you don't need the submodules

@colorpump
Copy link

@mode777
Thank you a lot. I forked it and looked a bit inside.

It seems to me actually almost complete. Maybe I can help.
I just wondered how to handle the submodules:

In your file I found

declare module "twgl.js" {
    // ...

    // draw module
        export module draw {
            export function drawBufferInfo(gl: WebGLRenderingContext, bufferInfo: BufferInfo | VertexArrayInfo, type?: number, count?: number, offset?: number): void;
            export function drawObjectList(objectsToDraw: DrawObject[]): void;
        }

        export function drawBufferInfo(gl: WebGLRenderingContext, bufferInfo: BufferInfo | VertexArrayInfo, type?: number, count?: number, offset?: number): void;
        export function drawObjectList(objectsToDraw: DrawObject[]): void;
  • I couldn't find any best practice how to handle sub modules in .d.ts files.
  • Do you have any tipps how to elminate redundancy?
    • otherwise it's also fine and I could continue in the same way

@colorpump
Copy link

@greggman back to mode777's original question 😉 ... Is it fine for you that he contributes the types to GitHub: DefinitelyTyped or would you like to do it as the owner of twgl.js?

Sorry for pushing so much 😄 ... I just think it would be a huge enhancement for twgl.js.

@mode777 I just don't really understand your point about 'owning' the repositories. Wouldn't you just fork DefinitelyTyped like 10 thousand others and make a pull request, and in the end it's part of @types? I'm sorry if I don't get something, I never contributed code by myself yet.

@mode777
Copy link
Author

mode777 commented Sep 5, 2017

@colorpump Not sure if I understand your question, but consuming the submodules will work just like in javascript:

import * as twgl from "twgl.js";

const info = twgl.primitives.create3DFBufferInfo(gl);

EDIT Sorry, now I get it: The redundancy here comes from twgl.js and is on purpose. That's why it is included twice.

@greggman: My opinion on this has changed. I think the definition files should be contributed together with the library in the same npm package. This seems to be the way many libraries are doing it (rxJS, electron, Angular...). If you agree, I will fork twgl.js add the definitions and submit a pull request.

@greggman
Copy link
Owner

greggman commented Sep 6, 2017

I haven't used TypeScript yet. Looking forward to it!

Do one of you guys mind forking twgl, adding in the typescript stuff, and setting up whatever other files are needed so I can see it work and compile?

In other words I want to be able to, load up a typescript based example in VSCode, see VSCode show the typescript completions for twgl, see a twgl example written in typescript, see it process the sample into JavaScript so I can run it.

I don't know what the best way to do that is. In a subfolder? Stuff in the root? In a separate project?

If any of you have time to do that it would be really helpful. I'd basically like to see a working example so I have something to go off of.

@colorpump
Copy link

As mode777 took the initiative and also did already a lot, I want to let him go first.

@mode777 if you are busy these days, and want me to do it, it would be no problem for me - on the contrary: I would like to be helpful.

My idea would be:

  • to fork the main project and create a new branch called "typescript-definitions"
  • insert the file /src/twgl.d.ts
  • insert in /examples/: typescript-compiled.html, js/typescript-compiled.js and js/typescript.ts
    • like examples/amd-compiled.html, examples/js/amd.js and examples/js/amd-compiled.js
  • modify package.json
    • add "files": ["dist/3.x/twgl.d.ts"]
  • modify /Gruntfile.js
    • insert in copy: {... :
        twgl.d.ts {src: ..., dest: `npm/base/${${verDir}}/twgl.d.ts` }
    

I don't think it's necesarry to have a typescript loader included. But if you think different, we have to ....

  • add to package.json "devDependencies":"ts-loader" (to compile the typescript example with webpack inside a Grunt task)
  • adjust the webpack config inside of Gruntfile.js that it builds the the typescript example as well when npm run build

I think it would be easier for people who are interested to install typescript globally and run on the personal computer: tsc or tsc -w to see the typescript example rebuilding.

What do you guys think?

@mode777
Copy link
Author

mode777 commented Sep 7, 2017

@colorpump This seems like a reasonable way to go. Ideally you would run npm install twgl.js, then import * from 'twgl.js' and get intellisense straight away. I think this should be the goal.

I'm actually quite busy at the moment, so if you want to finish the definitions (I just used the excellent documentation for it) it would be highly appreciated.
Would you want to set up the fork as well?

@greggman I understand, that you want to see it work in action. I can set up a demo once the definitions are complete. Even plain Javascript users could benefit from the definitions, as many IDEs like vscode can use them to enchance dev experience.

@colorpump
Copy link

Yes, of course. I start tomorrow.
I also set up the fork.

colorpump added a commit to colorpump/twgl.js that referenced this issue Sep 11, 2017
@sami-badawi
Copy link

I do not see the twgl.d.ts file anywhere.
Do I need to run a gulp build myself to create this?

@colorpump
Copy link

@sami-badawi Currently it's only in my fork repo under the branch feature-typescript-support.

I realized that the twgl functions and Type Definitions are constantly under development. So I'm trying to work on an automatic test, in order to keep the definitions file in sync with the library itself.

I'll keep you updated here.

@sami-badawi
Copy link

Thanks @colorpump
Last night I had made my own type definition based on work by @mode777 from this page https://gist.github.com/mode777/4f8eb25b123b2c5627034f797834f949
I placed it in:
node_modules/twgl.js/index.d.ts
This works but it seems a hackish and I am reluctant to check that into git my project:
https://github.com/sami-badawi/shapelogic-typescript
Is there a better way that I can take your file:
https://github.com/colorpump/twgl.js/blob/feature-typescript-support/src/twgl.d.ts
And use that instead?
Where should I place twgl.d.ts ?

@colorpump
Copy link

I would place your definition file in a source folder and reference it by

///<reference path="./pathToYour/twgl.d.ts"/>
import * as twgl from 'twgl.js';
  • befefit: it's not in the node_modules, which will be overriden when npm install
  • disadvantage: you have to do this in every module, which has twgl.js as dependency.

To avoid a reference tag in every module, I think you can use the "files" property in tsconfig.json.

But you could also wait, until I'm done and until hopefully it's part of twgl.js. Then you will not need anything besides import * from 'twgl.js' ... I hope 😉

@sami-badawi
Copy link

Thanks @colorpump. I followed your advice and added code.

///<reference path="./twgl.d.ts"/>
import * as twgl from 'twgl.js'

I am building with Webpack and got a compile error.

Cannot find type definition file for 'twgl.js'. (2688)

I changed the my code to:

///<reference path="./twgl.d.ts"/>
import '../node_modules/twgl.js/dist/3.x/twgl-full.js'
import * as twgl from 'twgl.js'

And still got same compile error.

Cannot find type definition file for 'twgl.js'. (2688)

Any ideas?

@colorpump
Copy link

I did the same thing, and it's working:

image

@sami-badawi
Copy link

Thanks @colorpump, my type definition is working fine, but I get a compile error when I call my build command:

webpack

When I installed the type definition file in
node_modules/twgl.js/index.d.ts
I got both working type definition and a working compiler. :D

Are you using Webpack too?

@colorpump
Copy link

yes, I tried the same example with webpack, and it did compile without errors.

I used webpack 2.4.1

But now I would say: You found a dirty solution. This should be enough until twgl.js comes with typescript definition. Once it's included, you won't have problems.

sami-badawi added a commit to sami-badawi/shapelogic-typescript that referenced this issue Sep 16, 2017
…rom this page https://gist.github.com/mode777/4f8eb25b123b2c5627034f797834f949

Tried to use more elegant approach suggested by @colorpump here:
greggman/twgl.js#50
This code can be backed out when type definitions are added to twgl.js
@sami-badawi
Copy link

I use Webpack 2.6, so I doubt that is the problem.

I found a slightly better solution. Instead of having the type definition live here:

node_modules/twgl.js/index.d.ts
It lives here
types/twgl.js/index.d.ts

And I have a npm task:

"type-copy": "cp -r types/ node_modules/"

If anybody else run into a similar problem before the twgl.js type definition gets rolled out, my code is here:
https://github.com/sami-badawi/shapelogic-typescript/tree/feature/twgl

@geon
Copy link
Contributor

geon commented Feb 8, 2018

@sami-badawi

I tried your type definition. I don't want it inside the node_modules folder, so I changed the first line from

declare namespace twgl {

to

declare module 'twgl.js' {

, removed the lines

export = twgl;
export as namespace twgl;

and added

{
	"compilerOptions": {
		"typeRoots": ["node_modules/@types", "./src/types"]
	}
}

to tsconfig.json.

@geon
Copy link
Contributor

geon commented Feb 8, 2018

Any progress on this?

@colorpump
Copy link

sorry, I'm out, as I got other projects.

I wanted to write automated tests for the definition file, so that tests would bark when new updates in the core are done without updating the definition file. But that was a bigger pain as I expected.

I hope someone can implement this feature. Otherwise I hope to find some time for it again this year - but I cannot promise.

@geon
Copy link
Contributor

geon commented Mar 20, 2018

FWIW, I am using the typings from @sami-badawi , with some tweaks. I added a couple of functions, and corrected the Arrays type.

https://github.com/geon/mem/commits/master/src/types/twgl.js/index.d.ts

@greggman
Copy link
Owner

I uploaded a new PR (just as an example). It's got @pineapplemachine 's PR in it. It also has a typescript example started. I don't know typescript at all and I don't feel like digging around trying to figure out how to get VSCode to see the type file when editing the example so I just pushed it here and maybe someone else more familiar with typescript can upload a working version.

What I want to happen

Load examples/textures-typescript.ts into VSCode. Hover mouse over twgl and see that VSCode see's it's the twgl module. Have it see other types etc...

What it does now

Hovering over twgl VSCode just says 'any'

Building

download the PR then

 npm install
 npm run build

Then, if you change something about how types.d.ts is made

 npm buildts   # remake dist/4.x/types.d.ts

To compile the .ts example

 ./node_modules/.bin/grunt ts   # build examples/textures-typescript.js from examples/textures-typescript.ts

The sample HTML file is in examples/textures-typescript.ts

Other issues

typescript by default has no knowledge of WebGL2 so loading types.d.ts into VSCode shows errors anywhere a webgl2 type shows up

@greggman
Copy link
Owner

Took @pineapplemachine 's latest and push a new PR here

#97

It builds and generates a .js file for the example, things seem to be typed in VSCode but the code doesn't run in the browser. It gets the error

textures-typescript.js:2 Uncaught ReferenceError: exports is not defined
    at textures-typescript.js:2

Searching online it's not entirely clear to me if I have to use webpack/babel/rollup to get it to work or what. Will look into it if/when I have time unless someone else knows how to fix it.

@greggman
Copy link
Owner

pushed 4.8.0 with .ts files

@pineapplemachine
Copy link
Contributor

I updated the dependency version and it's working great with my TS project that prompted me to come and talk about definitions in the first place. Heck yeah. I will certainly fix any errors that I come across, and if you @ me in any issues created by other devs in regards to the definitions I can take a look at them too.

@greggman
Copy link
Owner

There's still the whole ArrayBufferView issue which I'm not 100% sure what I want to do there. The problem with ArrayBufferView is that DataView is an ArrayBufferView but it's not compatible with most of the places that take an ArrayBufferView. I declared TypedArray inside primitives.js but not sure if I should use that other places yet or not or just don't care that passing a DataView will fail

@greggman
Copy link
Owner

englercj/tsd-jsdoc#89

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

No branches or pull requests

6 participants