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

Auto generation for typescript definitions #5317

Merged
merged 3 commits into from
Jan 11, 2019
Merged

Auto generation for typescript definitions #5317

merged 3 commits into from
Jan 11, 2019

Conversation

ivanpopelyshev
Copy link
Collaborator

@ivanpopelyshev ivanpopelyshev commented Jan 1, 2019

Work in progress.

npm install
npm run types

The result is inside dist/types folder.

Current problems:

  1. resource-loader dependency
  2. we have to separate canvas definitions from the rest
  3. private:true for jsdocs
  4. strange messages about module.exports
  5. module for static members of classes

@ivanpopelyshev
Copy link
Collaborator Author

OK, now we have only one problem before it generates correct typescript definition - static overrides, from and fromImage.

I'll add it to manual steps list.

Please review and merge this.

Copy link
Member

@GoodBoyDigital GoodBoyDigital left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Member

@themoonrat themoonrat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there something to be sorted for the future where anything 'protected' doesn't start with an underscore? Feels weird accessing properties that start with an underscore. Dunno if that would mess up tons of plugins and hacks tho

@bigtimebuddy
Copy link
Member

Please don’t merge this yet, I’m testing it today.

@ivanpopelyshev
Copy link
Collaborator Author

Manual changes list:

  1. add typings for resource-loader
  2. event-emitter
  3. change from and fromImage, add extra declarations in TilingSprite and CubeTexture.

I'm gonna commit typings based on that thing and manual changes in separate PR soon

@bigtimebuddy
Copy link
Member

Currently, Travis is blocked on getting the latest tsd-jsdoc published. There's critical fix in master not yet on NPM. Also @ivanpopelyshev created 2 PRs that would be helpful here (we could remove some hacks).

We have been testing typings in this branch of pixi-typescript. Seems to work well and support typings for legacy and default bundles.

@bigtimebuddy bigtimebuddy added this to the v5.0.0-rc milestone Jan 5, 2019
@englercj
Copy link
Member

englercj commented Jan 9, 2019

Just published tsd-jsdoc@2.1.1

Copy link
Member

@englercj englercj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we separate the jsdoc comment fixes from the type generation changes?

The jsdoc fixes should just go in ASAP, but we need to iterate on the typing generation process a bit. I don't want the later to hold up the former. It will also make review much easier.

package.json Outdated
"docs": "mkdirp dist && jsdoc -c jsdoc.conf.json -R README.md",
"types": "npm run types:legacy && npm run types:default",
"types:default": "mkdirp dist/types/pixi.js && jsdoc -c types/jsdoc.conf.json && node types/assemble pixi.js",
"types:legacy": "mkdirp dist/types/pixi.js-legacy && jsdoc -c types/jsdoc-legacy.conf.json && node types/assemble pixi.js-legacy",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it make more sense for each library to generate a type file for each package, so we can submit them to npm and reference them in the package.json files. Then have the bundles just collect and export the things it includes?

Having a pixi.d.ts file isn't very useful for those of use that use a couple packages piecemeal.

Copy link
Collaborator Author

@ivanpopelyshev ivanpopelyshev Jan 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes sense, however we have no userbase that use mini-modules yet, and there's no tutorial on how to make your own analogue of bundle.js that is required for that case.

Its optional.

Copy link
Member

@englercj englercj Jan 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have no userbase that uses individual packages yet, because they are not released.

Without doing it the way I'm describing it isn't even optional, it is just unsupported. If we are going to publish separate packages to npm (we are) then they should work.

Also, I definitely will be using individual packages and not the bundles.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to create typings for packages individually, but the problem is that the individual packages do not create a global object. So namespace doesn't exist. It would take longer to figure out that piece, but I'm fine having that be a future v5 release. For now, improving on the existing typing is a good step.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey guys!

I just ran into some typing problems with the new v5 release. We use TypeScript internally for everything so naturally having working types for Pixi is a must. One of the biggest cool new features for v5 is the individual packages which we can leverage to reduce our bundle size.

Unfortunately as discussed here there are no typings for them. It seems pixi-sound has ran into this issue as well since there is a "hackyish" workaround for it: https://github.com/pixijs/pixi-sound/tree/master/typings

On top of that I ran into some problems with the types in pixi-sound that I reported: pixijs/sound#107

So I guess my main question is that since pixi-sound and pixi-spine are already written in TypeScript, what is your general feeling towards it? I am playing around with the idea of doing a fork of Pixi to convert it to TypeScript and use the TypeScript compiler to auto-generate the declarations (with comments included so you get nice tooltips inside IDEs like Visual Studio Code). This is essentially how all of our internal packages work as well.

@ivanpopelyshev
Copy link
Collaborator Author

I can separate it, if it doesn't makes a problem for you to use them both at the same time to test it.

@englercj
Copy link
Member

englercj commented Jan 9, 2019

I can separate it, if it doesn't makes a problem for you to use them both at the same time to test it.

I'm saying we should PR the jsdoc comment changes and merge them, then look at (and test) the type changes separately. Should be easy to test the type changes because we should've already merged the comment changes if we are doing that.

@englercj
Copy link
Member

englercj commented Jan 9, 2019

The jsdoc comments have been merged into dev, lets rebase this to include only the type generation changes.

Copy link
Member

@englercj englercj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for setting this up, and looks pretty good to me.

Only comments I have left are that I don't think we need the extra definitions files for resource-loader and eventemitter3, since they already exist in the node_modules folders. We could either load them and write then in the file you are building with the script, or let typescript handle them with an import in the type definitions.

types/loader.d.ts Show resolved Hide resolved
declare namespace PIXI {
namespace utils {
// https://github.com/primus/eventemitter3
export interface EventEmitter {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this needed? Couldn't you just have the script load the typings in node_modules/eventemitter3/index.d.ts?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pixi classes extend PIXI.utils.EventEmitter and that class is defined in utils through jsdoc. The other thing is that interface.

I can make script that transforms EventEmitter3 typings to what we need, but I think that its over-complicating things.

Copy link
Member

@englercj englercj Jan 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The thing in utils is just ee3 unchanged:

import EventEmitter from 'eventemitter3';
export { EventEmitter };

Why can't this file be replaced by the assemble script just writing to the output:

declare namespace PIXI {
    namespace utils {
        /* insert loaded node_modules/eventemitter3/index.d.ts */
    }
}

Then this file doesn't need to exist.

Copy link
Member

@englercj englercj Jan 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Specifically I mean change this line:

const eventTypes = fs.readFileSync(path.join(__dirname, 'events.d.ts'), 'utf8');

To be this:

const eventLibTypes = fs.readFileSync(path.join(__dirname, '../node_modules/eventemitter3/index.d.ts'), 'utf8');
const eventTypes = `declare namespace PIXI {\n    namespace utils {\n        ${eventLibTypes}\n    }\n}\n`;

And then delete this events.d.ts file.

Copy link
Member

@bigtimebuddy bigtimebuddy Jan 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, it's not that easy. The typings from EventEmitter have an export as namespace EventEmitter; line that needs to be removed (not a big deal tho). And, more importantly, the existing events.d.ts contain some important hand-rolled information about the event types for DisplayObject and Container. It doesn't look as if tsd-jsdoc handles these events.

I think it's acceptable as is. We can polish this over time. There are a lot of hacks in the types/assemble.js file that we should try to weed-out.

Copy link
Collaborator Author

@ivanpopelyshev ivanpopelyshev Jan 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

autogen based on someone else definitions for small library looks like a leftpad for me.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The typings from EventEmitter have an export as namespace EventEmitter; line that needs to be removed (not a big deal tho).

Good point, this is why I feel we should be using import rather than generating single-files, but we can do that in the future.

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.

6 participants