Skip to content
This repository has been archived by the owner on Feb 23, 2023. It is now read-only.

Merge with Pixi.js #72

Closed
bigtimebuddy opened this issue Jul 12, 2016 · 31 comments
Closed

Merge with Pixi.js #72

bigtimebuddy opened this issue Jul 12, 2016 · 31 comments

Comments

@bigtimebuddy
Copy link
Member

Issue brought up in #58, moving forward, we should move the PIXI typings to the main pixi.js repo. That way the typings get locked to the distributed version of the library. Also, would ensure that when developers make PR against pixi.js that change the API, they would also need to make the corresponding API change in the typings. Thoughts?

@clark-stevenson
Copy link
Collaborator

clark-stevenson commented Jul 12, 2016

Hey!

I would really appreciate guidance on this one from yourself and those of us here. I have almost no exposure to the mechanisms involved with typing's or anything else so I have very little opinion to offer.

@staff0rd @whizzkid @webbiesdk ?

@bigtimebuddy
Copy link
Member Author

The mechanism is pretty simple in newer versions of TypeScript: basically you add a typings field in the package.json which is used when building with TypeScript to resolve all the module's types.

This could be achieved with a simple PR to Pixi.js. The tricky thing here is that we'd need to add typings for all the plugins into their respective plugins (e.g. pixi-spine, pixi-particles, etc). Would require a little coordinate, but could be ace at resolving versioning issues moving forward.

Anyways, I'm pro-TypeScript and would love to see it become a first-class workflow for Pixi.js developers.

@englercj
Copy link
Collaborator

If we are talking about the future of this repo, I vote for tool-generated typings instead of manually managed ones!

A start: https://github.com/englercj/tsd-jsdoc

@staff0rd
Copy link
Collaborator

Tool-generated typings (hopefully including comments) is a definite plus, however it should be a separate issue to this one (which it is).

Regarding repo-merging, specifically the plugin merges, to my knowledge this would only be possible via Typescript 2.0 (ie; definition augmentation?). If so, the question is should this issue then be put on hold until Typescript 2.0 is out of beta, and, are there any cases (backwards compatibility or otherwise) where users would not update to Typescript 2.0 thereby still requiring access to non-augmented type definitions?

@webbiesdk
Copy link
Contributor

Don't know if this belongs here or in #70, but the discussion seems to be happening here, so I'm writing it here.

I don't think tool generated declaration files are the way to go.
Generated declarations are certainly cool, and can be used as a helpful tool when writing declaration files. (As an example, it could be used to generate a complete list of changes between two versions)
But JSDoc and TypeScript cannot handle every situation (although TypeScript seems to be trying hard lately), and sometimes things are best if handled slightly different in JSDoc and TypeScript.

(examples are from v.4).

TL;DR: JSDoc and TypeScript are not the same. Treat them as such.

First example, one-time use interfaces (look at the BitmapText constructor).
In JSDoc, if you have a function which takes an object with some fields. You normally write the documentation for the object directly in the documentation for the function.
But in TypeScript this is usually exported into a separate interface, because inline object-definitions look ugly in TypeScript.
If the documentation was generated by a tool, this tool would only generate a good looking declaration file if it knows to "export" the object into a separate interface, and knows what name to give the interface (where would that come from?).

Second example, the CONST module.
In the current declaration file, CONST is documented as a module inside the PIXI module.
But since the properties are actually all accessed directly on the PIXI module, the fields are duplicated in the PIXI module (using typeof to type the properties).

There are two alternatives to this:
1: Not have a CONST module, and only have the fields on the PIXI module.
This would work, but then the documentation would no longer contain a CONST module.

2: Actually use mixins.
But this requires that the PIXI object is an instance of an interface. In which case we would lose the pretty tree-structure of the current declaration file. Which makes the declaration file harder to navigate.

The current JSDoc documents it like the first alternative, and this is also the best way to document it using JSDoc.
And a tool converting JSDoc into TypeScript would therefore use the first alternative. Removing any choice.

Third example: TilingSprite.fromImage and TilingSprite.fromFrame (@clark-stevenson should know this one)
TypeScript likes to think that classes inherit static methods (which is the case in TypeScript, but not in all of JavaScript).
But when you have a class structure where this isn't the case (like in PIXI), this generates problems.

In this case the problem was solved in a really unclean way, but with a comment stating so, which means a reader of the declaration file shouldn't be confused as to what the actual meaning is.
And I think it is better to have a human make these choices.

Fourth example, look at PIXI.GroupD8
In the JavaScript file, most of the methods and fields are left undocumented.
Most likely because it would clutter up the .js file, it one had to write a detailed description for every single field.
And this is OK.
But if a tool was introduced, then in order to have any documentation of GroupD8, the JavaScript file would have to include detailed type descriptions of every field, which would only help the tool but not anyone else.

There are more examples (like the pixi (in lowercase) object), but this is long enough already.

These could most likely be fixed by some custom annotations in the JSDoc.
But then one would be writing documentation for the tool, and not for the human.

But this is not my repo, and in the end it is your decision.
I'm trying to say that using tool-generated declarations might make publishing new versions easier.
I just think that the end result would suffer.

@clark-stevenson
Copy link
Collaborator

Move Pixi to TypeScript would work.... lol 😛

I am moving at a Glacial pace on this front. Last couple of weeks, I have no build and no errors so progress is pretty much zero. Anyway aside from all of that, I have future concerns. Most of them are highlighted above. Interfaces are not real things, Dependencies can be a problem too. PIXI without EventEmitter sucks. Either dependencies would magically work, they would need to be maintained, or there might actually be 2 styles of JSDoc. (Everyone appears to use a different style/guideline for how to comment code) .

I really hate the primitive task of sitting on Github reviewing every PR tho. I was really enticed by #70

@clark-stevenson
Copy link
Collaborator

It is likely going to be the case that these definitions for v4 will be for TypeScript 2.0 only since I see no reason to wait around now that it is final.

Need to spend some time wrapping my head around all this but just thought I would share.

@paralin
Copy link

paralin commented Sep 23, 2016

I'm happy to help with this, I just downloaded this repo to start working on v4 definitions, but if you guys have a better plan I'd love to help us get there asap (using Pixi in a project right now)

@webbiesdk
Copy link
Contributor

The TypeScript 2 release is very interesting, but unfortunately it is not something this repository will necessarily gain from.
As i understand it, the @types package on npm pulls mainly from DefinitelyTyped, so people running npm install -s @types/pixi.js will get the version on DefinitelyTyped, and not the one from this repository.

So now there is a greater need for the DefinitelyTyped version of pixi.js.d.ts` to be updated (any volunteers for that merge?).

One solution is to include the declaration file with the main repository by using the typings field (as suggested further up). Then there is no need for developers to download the version of @types.
And I believe it doesn't require any modifications to the declaration file. As long as people loads one of the pre-build versions of PixiJS.

Another solution is to delete this repository, and move everything to the DefinitelyTyped project (you can have versioned declarations, like the hammerjs declaration has).
I'm guessing that is not an option.

The best solution would be to somehow get the @types package to point to this rep, but that seems impossible.

@paralin
Copy link

paralin commented Sep 26, 2016

If you merge an official typescript definitions file into the pixi repo the definitely typed people will mark pixi as deprecated so users get a warning that they don't need it anymore. Also, users would automatically get the typescript definitions working when they updated pixi to the latest, which would produce warnings if they had the old @types installed.

Best approach remains merging this into pixi and/or rewriting pixi in typescript.

@webbiesdk
Copy link
Contributor

We don't need to merge the TypeScript declaration into the pixi repo, some build-script could fetch the latest version from this repository, that way the two repositories could remain separate.

@paralin
Copy link

paralin commented Sep 26, 2016

Makes no sense to do that, what's the advantage over just merging?

@webbiesdk
Copy link
Contributor

This repository has a couple of branches, one for each major version of PixiJS.
If people are using an old version of PixiJS and want to get a declaration file for it, then they can get it here.
Moving the development of the declaration file into the main repo would somewhat remove that.

Keeping things in separate repositories also helps keeping the two things separate. Everything related to PixiJS and TypeScript is in this repository, while the main repo deals exclusively with JavaScript.

@andrewstart
Copy link
Collaborator

Moving all further Typescript definitions (keeping old versions here) into the main repo would have other benefits, like being able to easily develop a feature branch with its typings, and anyone that checks out that branch and symlinks it will get the full typings.

@staff0rd
Copy link
Collaborator

I think the version branches on this repo are misleading, for example you need master for pixi-spine, but v4 for pixijs, even though both are v4. Merging with the main repo is the way forward I think, however I'm still not sure if its possible (even in typescript 2) to augment existing definitions in the case of plugins that extend base classes, for example pixi-display.

@webbiesdk
Copy link
Contributor

It is possible to augment existing declarations with new ones that extend base classes.
But it requires a lot of refactorization (and it's not pretty), I got an example of how it can be done below.

declare module PIXI {
    interface ShaderConstructor {
        new (): PIXI.ShaderInstance;
    }
    interface ShaderInstance {
        baseMethod(): number;
    }
    export var Shader: ShaderConstructor;
}

// Extended
declare module PIXI {
    interface ShaderInstance {
        fromExtended(): string;
    }

}

var num: number = new PIXI.Shader().baseMethod();
var str: string = new PIXI.Shader().fromExtended();

And you guys convinced me about the merging with main repo thing. I now agree the best thing is to move into the main repo.

@staff0rd
Copy link
Collaborator

So its basically shelving all classes for interfaces?

@webbiesdk
Copy link
Contributor

Yes.

As I said, it's not pretty.

@clark-stevenson
Copy link
Collaborator

clark-stevenson commented Sep 27, 2016

The best solution would be to somehow get the @types package to point to this rep, but that seems impossible.

😞

Well there goes my plans.

Another problem is that V4 was originally a totally different repo, and then V3 became V4 at a unknown point in time. An offshoot of that is not really knowing at what point other libraries (such as Spine) adapted so that is why there is currently 2 branches but v3 is pretty much abandoned.

I have no preference at all. To me all that really matters is that there is 1 place where TS dudes go to access Pixi. There seems agreement adding it to the official repo is the way forward and others are way way more qualified than me on the pros and cons of that and the tooling surrounding how to best achieve that end.

So long as I can make a weekly PR or so (and the guys above correct my mistakes lol). I am a happy guy.

@paralin
Copy link

paralin commented Sep 27, 2016

@clark-stevenson Weekly PR to the main repo, put pixi.d.ts next to pixi.js, then in the package.json:

"main": "./dist/pixi.js",
"types": "./dist/pixi.d.ts"

Simple as that. Then all anyone has to do is npm install pixi.js and the typings just work, no matter which version you installed.

@webbiesdk
Copy link
Contributor

Move Pixi to TypeScript would work.... lol 😛

And why not actually do it?

Most of PixiJS is pretty straightforward to convert to TypeScript.

Without having looked too much at it, it seems that the only non-trivial conversion are type-annotations, and mixins, and mixins are only used for the core object and the DisplayObject class.

I might be looking into converting stuff from JavaScript to TypeScript in general, and PixiJS would be a nice case.

@ivanpopelyshev
Copy link
Contributor

Guys, i need help with https://github.com/pixijs/pixi-spine . I use "tsify" to compile it, but the only way to get headers is "tsc -p tsconfig-2.json". It produces config without namespaces, and I really dont understand what should I do with it.

@clark-stevenson
Copy link
Collaborator

Sorry I can't help Ivan. I wasn't sure myself when I looked at it.

Nice to see you using TS 👍

Also thanks @paralin

I have another related question for the good folks here 😄

Inside the definition file right now, is this https://github.com/pixijs/pixi-gl-core

So would that mean, that pixi-gl-core would get extracted from the current definition and added to that project too? And if that is the case, how is the dependency handled between these? The fact that pixi.js.d.ts requires pixi.gl.core.d.ts

@ivanpopelyshev
Copy link
Contributor

@clark-stevenson i have to use TS because official EsotericSoftware runtime is TS now :)

@staff0rd
Copy link
Collaborator

staff0rd commented Oct 6, 2016

I'm of the opinion that plugin definitions don't belong in the main definition. While it's easy-ish to include the 'official' ones, it doesn't help others build typescript definitions for their own non-official plugins. The same too for dependencies - they should be split out and required/imported as needed (I'm not actually sure whether this is possible, although this looks like it might help).

Were (some/all/which?) plugins to stay within the main definition, and that definition were then merged into the main pixi repo, then the main repo would also have to be responsible for reviewing/merging type definitions for those chosen plugins. As such I think that the decision on whether to merge with the main repo should be based on whether plugins and dependencies can be split out.

While it's a bit verbose, and would take some effort, it looks like @webbiesdk's example is currently the only way to do this, considering there doesn't seem to be any traction on @clark-stevenson's issue here.

@ryanlangton
Copy link

What is the best way to get the most current typings for pixi.js? I have the following in my package.json:

 `"@types/pixi.js":` "4.3.1"`

But it seems these typings are missing PIXI.Application. Should I just get the typings file manually from this repo and include it in my project?

@cursedcoder
Copy link
Collaborator

@kimhw0630
Copy link

seems like I can't install PIXI typescript above v.4.3.2..

npm install
npm ERR! Darwin 16.4.0
npm ERR! argv "/usr/local/bin/node" "/usr/local/bin/npm" "install"
npm ERR! node v7.4.0
npm ERR! npm v4.0.5
npm ERR! code ETARGET

npm ERR! notarget No compatible version found: @types/pixi.js@4.4.0
npm ERR! notarget Valid install targets:
npm ERR! notarget 4.3.2, 4.3.1, 4.3.0, 4.1.0, 3.0.34, 3.0.33, 3.0.32, 3.0.31, 3.0.30, 3.0.29, 3.0.28, 3.0.27, 3.0.26, 3.0.25-alpha, 3.0.24-alpha, 3.0.23-alpha, 3.0.22-alpha, 3.0.21-alpha, 3.0.20-alpha, 3.0.19-alpha, 3.0.14-alpha, 3.0.13-alpha
npm ERR! notarget
npm ERR! notarget This is most likely not a problem with npm itself.
npm ERR! notarget In most cases you or one of your dependencies are requesting
npm ERR! notarget a package version that doesn't exist.
npm ERR! notarget
npm ERR! notarget It was specified as a dependency of 'live-dashboard'

@clark-stevenson
Copy link
Collaborator

clark-stevenson commented Mar 11, 2017

I don't know what it means tbh.

Ivan made a PR at the end of Jan which just got merged on DT. It might solve the issue and he did a lot of the grunt work. 👍❤️

There is one more PR needing made to DT to bring us into place. It is Application, which has changed most in this time but is one of the best things added for dev usability!

@englercj
Copy link
Collaborator

v4.4.0 just made it to npm: https://www.npmjs.com/package/@types/pixi.js

@clark-stevenson
Copy link
Collaborator

clark-stevenson commented Jun 10, 2017

So guys,

I think that this will continue to be manual. The history of the file is pretty rich and it has had multiple homes. Phaser born the definition years ago where it branched off (you can see me begging in the forum when I had 1 post for a def).

I am pretty happy with the definition, I mean, it is pretty accurate, and I absolutely hate coming here and looking over commit histories, but in the end.... It takes no more than an hour and over time, all of the kind folks have helped make it pretty accurate today.

Furthermore there is stuff in the definition that doesn't belong there, half the dependencies are in there.... and splitting these up, the logistics of it, would be incredible and the tedium infinite.

So for lack of a better plan, I am going to continue doing stuff here as usual, and pushing to DT with significant releases.

If there is a V5 of Pixi, TypeScript should be considered and failing that, I would be interested at looking at creating a TS fork as it would be a hell of a lot more interesting than studying APIs!

Anyway thanks for everyone answering all my questions in commit-comments about stuff over the past year or so, I learned a lot and plan more I am sure! 💃

Especially that the PR I made tonight to DT, was around 6 months old and honestly.... really not that much changed. A testament to strength of the API. 👍

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

No branches or pull requests