-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Generate TypeScript definitions #2279
Conversation
I have a heavily modified version of that script that handles some of the typescript overloads and missing types better... |
@Spongman it would be awesome if you could share the code here so I can incorporate it 😊 |
yeah, i need to clean it up first so it fits better with what you have. btw: here's what it generates: https://p5ide.azurewebsites.net/assets/p5.d.ts |
ok, i'm sorry, i have no idea how to get github to send a PR from my fork to yours, but here's my branch: |
Awesome @Spongman! I cleaned up your commits, which I hope is OK with you. I think this is ready to merge now, anyone I need to ping? |
i would exclude the p5.sound stuff from this PR as that's already been merged in another PR. |
@Spongman anything else we need? |
yeah, you might want to merge with my fork again: https://github.com/Spongman/p5.js/tree/typescript. i added the param & return descriptions to the .d.ts doc comments. i have no idea how to send a PR directly between different forks. |
@Spongman time to bother you again 😁
|
@Spongman fixing the MISSING definitions solved the rest of the problems. I don't think there is a pretty solution to those definitions so I'm OK with hard-coding them. Merge ready? |
it might be worth looking into how these would be deployed. there's some grunt scripts to do the deployment - see if you can work out how to place these alongside the .js files... |
One thing I have noticed playing around with this script & others (at least last I checked, so forgive me if this has been resolved) is that it doesn't seem to include type definitions for the p5 constructor in instance mode, even though the constructor specs do seem to be in the json YUIDoc spits out. |
i fixed the missing p5 constructor in two commits: |
I had a look at the grunt tasks. What do you think? |
yeah, i think adding them to the github release makes sense. (ec7d563)
|
ok, so it's not just my PRs that are failing eslint:test on travis. do the tests pass on your local machine? |
They did not pass on my local machine, but the problem was still introduced by changes on master. Master has fixed that now though, so tests pass on my machine again. |
Hey there, just wanted to pop in and ask how this project was going. I've been using p5 in JavaScript for awhile now, and would love to see a definition file created for it. |
Check my comment here for a recent .d.ts file: #1339 (comment) |
Improves error reporting. Correctly emits readonly properties.
Tone.Signal is a type from another library so we can't "see" its type. It's only used as a return from a constructor, but constructors can't have return types in TS so we don't even emit a definition for it. SoundObject is either p5.Sound or an object that has .connect(unit) (e.g. Amplitude, AudioNode). YUIDoc doesn't seem to have free-standing interfaces so expressing "has .connect" seems hard.
this is fantastic. sorry it took so long to review. |
Is there any guidance documentation for getting started with TypeScript on p5? |
@dgennetten we have gathered some example projects in ps.ts. I hope they can get you started! Otherwise, open an issue over there. |
I've added a grunt-task that generates TypeScript definitions from the YUIDoc. It's probably not perfect, but on a glance it looks fine.
It uses the code by @toolness from http://processing.toolness.org/general/2016/03/16/typescript-to-the-rescue.html and depends on and includes the fixes in processing/p5.js-sound#222.
Types that can't be modelled using YUIDoc could be manually modified using declaration merging.
I can maintain the type-generation in the future if there are missing features / enhancements needed.
Closes #1392