Convert core to typescript #6340

ivanpopelyshev · 2020-01-17T14:07:12Z

Its done. Lets review it. Discussion and a few details about conversion is in the post below.

bigtimebuddy · 2020-01-17T14:35:10Z

Good start

bigtimebuddy · 2020-01-19T15:56:01Z

My one request is you do-no-harm with this PR. Keep the changes as straightforward as possible and no refactoring.

There’s always time to improve later.

ivanpopelyshev · 2020-01-19T16:54:48Z

Well, there is one small deprecation: I removed CubeTexture because it has from method that is not compatible with TS overrides. Easier to deprecate that than to add all necessary declarations.

ivanpopelyshev · 2020-01-19T18:33:27Z

OK, it should build, but I don't know how to deal with .glsl files. Please help!

bigtimebuddy · 2020-01-19T20:55:54Z

@Zyie could use your help on this one

Zyie · 2020-01-19T21:03:00Z

@ivanpopelyshev I believe this should do the trick.

Add this to the the global.d.ts file

declare module '*.frag' {
    const value: string;

    export default value;
}

declare module '*.vert' {
    const value: string;

    export default value;
}

ivanpopelyshev · 2020-01-19T21:50:11Z

OK, its building but now I have to fight typescript-eslint and few of those rules are just stupid.

   23:23  error    Identifier 'WEBGL_draw_buffers' is not in camel case              @typescript-eslint/camelcase
   24:24  error    Identifier 'OES_texture_float' is not in camel case               @typescript-eslint/camelcase
   25:23  error    Identifier 'WEBGL_lose_context' is not in camel case              @typescript-eslint/camelcase
   26:29  error    Identifier 'OES_vertex_array_object' is not in camel case         @typescript-eslint/camelcase
   27:32  error    Identifier 'EXT_texture_filter_anisotropic' is not in camel case  @typescript-eslint/camelcase
   28:30  error    Identifier 'OES_element_index_uint' is not in camel case          @typescript-eslint/camelcase
   29:24  error    Identifier 'OES_texture_float' is not in camel case               @typescript-eslint/camelcase
   30:30  error    Identifier 'OES_texture_float_linear' is not in camel case        @typescript-eslint/camelcase
   31:28  error    Identifier 'OES_texture_half_float' is not in camel case          @typescript-eslint/camelcase
   32:34  error    Identifier 'OES_texture_half_float_linear' is not in camel case   @typescript-eslint/camelcase
   33:28  error    Identifier 'WEBGL_color_buffer_float' is not in camel case

codecov-io · 2020-01-22T16:10:16Z

Codecov Report

Merging #6340 into dev will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff          @@
##             dev   #6340   +/-   ##
=====================================
  Coverage   74.7%   74.7%           
=====================================
  Files        105     105           
  Lines       6053    6053           
=====================================
  Hits        4522    4522           
  Misses      1531    1531

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8370b6e...55282a6. Read the comment docs.

ivanpopelyshev · 2020-01-22T16:26:14Z

OK, i have a few things:

I've tried to preserve everything even if I saw places where we could do better with TS. only in few cases I actually changed code a bit.

I tried to preserveprivate,protected and readonly modifiers but they are real pain. Maybe I'll remove a few of them to make code better
castToBaseTexture method exists to drop several occurences of texture || baseTexture in places that are not performance-important so one more function call surely wont be bad.
I prefer to write Array<something> for arrays with heavy usage of push/pop, and something[] for constant arrays. Yes, they are the same in TS, but I like that detail from other languages, like ArrayList from java and List from C#. However for experssions like f(x:number[][] = []): number[][] i prefer [] because its short.
Unused parameter requires 5 lines to disable eslint. What if we remove that requirement either from tsconfig? That will drop it to only one line.
autoDetectRenderer() returns Renderer. I dont know whether we want CanvasRenderer there too. I believe it can be addressed in canvas-PR.
CubeTexture was rarely used trash that had static from method with signature different from BaseTexture. I deprecated it.

Zyie · 2020-01-22T17:59:11Z

Unused parameter requires 5 lines to disable eslint. What if we remove that requirement either from tsconfig? That will drop it to only one line.

I think you can add an _ to the start of a parameter and typescript will understand that its an unused variable.

// eslint-disable-next-line @typescript-eslint/no-unused-vars
render(_object: any): void
{
}

This then makes it only one line

OK, its building but now I have to fight typescript-eslint and few of those rules are just stupid.

I agree think we should disable the @typescript-eslint/camelcase rule

bigtimebuddy · 2020-01-22T18:52:49Z

@EvidentlyCube could you review this?

EvidentlyCube · 2020-01-22T19:41:20Z

@bigtimebuddy I'll review it tomorrow!

Zyie

Wow this is huge, well done @ivanpopelyshev, very impressive.

One thing I've noticed is that this PR has a very different style to the other typescript conversion pr's.

Previously all public variables declared as public
static variables were defined at the top of the class not the bottom
variables were organised by public, protected, private

Not sure how much you guys care about the consistency but i thought i would mention it

packages/core/src/AbstractRenderer.ts

packages/core/src/Renderer.ts

packages/core/src/batch/AbstractBatchRenderer.ts

packages/core/src/textures/resources/VideoResource.ts

packages/core/src/textures/resources/SVGResource.ts

Zyie

Wow this is huge, well done @ivanpopelyshev, very impressive.

One thing I've noticed is that this PR has a very different style to the other typescript conversion pr's.

Previously all public variables declared as public
static variables were defined at the top of the class not the bottom
variables were organised by public, protected, private

Not sure how much you guys care about the consistency but i thought i would mention it

Zyie · 2020-01-22T22:41:58Z

packages/core/src/textures/resources/autoDetectResource.ts

@@ -57,7 +59,7 @@ export const INSTALLED = [];
 *        texture should be updated from the video. Leave at 0 to update at every render
 * @return {PIXI.resources.Resource} The created resource.
 */
-export function autoDetectResource(source, options)
+export function autoDetectResource(source: any, options: any): Resource


The options here are not any

We only can do something like {[key: string: any}, because we don't know the resources that user adds there.

Yes we do. The options are in the documentation.

We can add those fields to satisfy autocomplete but we need `{[key:string]:any} just because its possible to use in custom resources.

We should define strict types and then add a generic type for extensibility. Just using the generic type isn’t that great

I dont care about this place and it can be patched later when we find a case it doesnt work. Its still experimental API :)

You may change it and and other related places, and commit to this branch.

Zyie · 2020-01-22T22:42:42Z

packages/core/src/textures/resources/CubeResource.ts

-    constructor(source, options)
+    items: Array<BaseTexture>;
+
+    constructor(source: Array<string|Resource>, options?: any)


The options here are not any

they are being passed to autoDetectResource, we only can do something like {[key: string: any}

You need stricter types here for options. My suggestion is to make CubeResourceOptions and then combine all the options into the autoDetectResource: CubeResourceOptions|CanvasResourceOptions|VideoResourseOptions

^^^ its up to you. However I remind you that "|" might have side effects. I remember our struggle with Point|ObservablePoint ;)

Yeah that’s fine in this case because these Resource-specific options are non-functional and don’t have method signatures like Point v ObservablePoint.

packages/core/src/Renderer.ts

ivanpopelyshev · 2020-01-23T06:17:43Z

OK then i'll fix it and add public to fields that are public according to jsdoc and common sense.

Extra information.

Both I and @GoodBoyDigital have custom scene trees for pixijs. Its better if @pixi/core does not depend on @pixi/display.

Part of that is typings-related - that's why I made IFilterTarget, IMaskTarget, ISpriteMaskTarget.
However there's dependency from Renderer/AbstractRenderer classes that require code and API change, and that will be addressed in separate PR.

ivanpopelyshev · 2020-01-23T08:32:19Z

OK, I'm applying your idea regarding public/protected/private . However I leave the fields that have mixed usage without any modifiers. I also remove readonly in many cases, because otherwise core code will look bad.

We still have those modifiers in jsdoc, so we can apply them if TS will allow us to have custom modifiers.

This requires one more pass over all the code.

ivanpopelyshev · 2020-01-23T08:37:47Z

Regarding statics - I left them where they were originally and our team expects them there. If we move stuff like from to the top of the file, it will take attention of user instead of actual constructor.

ivanpopelyshev · 2020-01-23T09:46:40Z

Wow this is huge, well done @ivanpopelyshev, very impressive.

Thank you! I have predisposition for large and brain-melting tasks, but I couldn't have a chance to show that without people who "prepare the field" for me.

ivanpopelyshev · 2020-01-23T13:13:31Z

OK, access modifiers are ready.

ivanpopelyshev · 2020-01-23T14:09:30Z

Btw, @Zyie, I had to rename all run to emit because we dont have typings for run in runner, and maybe we have to ask @GoodBoyDigital how to do it, its his place.

ivanpopelyshev · 2020-01-26T10:53:05Z

I think we can merge this thing and treat autoDetectResource and IRendererContext improvements in separate PR.

Right now the only serious thing here is IRendererContext name, should we rename it?

bigtimebuddy · 2020-01-26T14:57:39Z

I like IRenderContext

EvidentlyCube

Summary:

There are a bunch of conversations marked as resolved but they don't seem to involve the code actually being changed
There is a lot of inconsistency in when access modifiers are applied. I personally prefer to always have them explicit, but for me also any approach is fine provided it's standarized across the whole codebase :). I suggest adding eslint rule to cover that.
Ditto for Array<X> vs X[]
One more thing I noted, there are a bunch of {[key: string]: T}} in the code, how about introducting a generic type type Dictionary = {[key: string]: T}`?
95 files is a lot, if I sound dry in my comments I didn't intend to!

And a suggestion for the future - when enormous modules like this one are converted to TS it might be a good idea to split it into smaller steps, maybe 20 files each. I have no hope of being able to go through 95 files and remain concentrated :).

EvidentlyCube · 2020-01-27T08:15:56Z

packages/core/src/AbstractRenderer.ts

+    protected readonly _tempDisplayObjectParent: DisplayObject;
+    protected _backgroundColor: number;
+    protected _backgroundColorString: string;
+    _backgroundColorRgba: number[];


In a different file you declare private explicitly, shouldn't that be followed here?
Also, it'd probably be a good idea to enforce explicit access modifiers in eslint if that's the way to go.

That particular field is used in RenderTextureSystem. I've decided not to add modifiers that can affect code readability.

packages/core/src/AbstractRenderer.ts

packages/core/src/Renderer.ts

packages/core/src/batch/AbstractBatchRenderer.ts

packages/core/src/batch/BatchSystem.ts

EvidentlyCube · 2020-01-27T08:28:12Z

packages/core/src/filters/Filter.ts

    {
        // do as you please!

-        filterManager.applyFilter(this, input, output, clearMode, currentState);
+        filterManager.applyFilter(this, input, output, clearMode);


Why was this code changed?

because that parameter is absent in filterManager.

packages/core/src/framebuffer/FramebufferSystem.ts

packages/core/src/geometry/Buffer.ts

packages/core/src/geometry/Geometry.ts

packages/core/src/renderTexture/RenderTexturePool.ts

ivanpopelyshev · 2020-01-27T11:29:03Z

@EvidentlyCube I forgot to push it :)

ivanpopelyshev · 2020-01-27T11:39:12Z

And a suggestion for the future - when enormous modules like this one are converted to TS it might be a good idea to split it into smaller steps, maybe 20 files each. I have no hope of being able to go through 95 files and remain concentrated :).

I feel like I'm rushing things, but from other point of view - we cant merge other PR's related to core while this one exists. Glory to our reviewers!

packages/core/src/textures/TextureSystem.ts

bigtimebuddy · 2020-01-29T23:21:23Z

While not perfect, I think this is good enough to merge. Further fixes can be added to address some of the outstanding things. Are you good @ivanpopelyshev or are you waiting for any other responses?

ivanpopelyshev · 2020-01-30T08:38:11Z

There are two possible improvements and one potential problem we can address in PR's.

IRendererContext improvements - WebGL1 + polyfill
autoDetectResource typings
The problem with castToBaseTexture that is actually a problem with custom filters and other stuff - I need to confirm it before fix.

Summary: YES, we can merge it.

ivanpopelyshev · 2020-01-30T12:58:09Z

Actually, we cant merge it. There are failing examples, i didnt test everything :(

bigtimebuddy · 2020-01-30T16:24:27Z

What’s broken? Could you keep me posted.

ivanpopelyshev · 2020-01-30T21:04:49Z

Graphics, both batched and main versions. Probably something with Geometry.. https://www.pixiplayground.com/#/edit/jPgE3ZEUhhVb7GJx9DuJb

…e for future people

ivanpopelyshev · 2020-01-31T13:20:56Z

OK, I've checked all examples. The one that doesnt work - ParticleContainer - fixed in #6371

core js->ts wip

60f8cc5

ivanpopelyshev force-pushed the dev-core-typescript branch from b54339e to 60f8cc5 Compare January 22, 2020 13:34

Passing tests

e881156

ivanpopelyshev marked this pull request as ready for review January 22, 2020 16:26

bigtimebuddy requested a review from Zyie January 22, 2020 16:31

Zyie requested changes Jan 22, 2020

View reviewed changes

applying a few @Zyie fixes

0501766

more modifiers

4ba7bcf

complete access modifiers

e177228

addressing almost all @Zyie comments

d7bf600

Zyie approved these changes Jan 26, 2020

View reviewed changes

ShukantPal self-requested a review January 26, 2020 23:39

ShukantPal approved these changes Jan 26, 2020

View reviewed changes

EvidentlyCube reviewed Jan 27, 2020

View reviewed changes

push the change

aeb5eec

Array -> [] for baseTexture, maxTextures.

20fbdd2

bigtimebuddy added this to the v5.3.0 milestone Jan 27, 2020

Merge branch 'dev' into dev-core-typescript

8370b6e

Zyie mentioned this pull request Jan 29, 2020

Add eslint rule for explicit accessibility modifiers #6361

Closed

1 task

ivanpopelyshev commented Jan 29, 2020

View reviewed changes

packages/core/src/textures/TextureSystem.ts Show resolved Hide resolved

Merge branch 'dev' into dev-core-typescript

850c7f1

ivanpopelyshev added 2 commits January 31, 2020 11:38

small clearmode jsdoc fix

55282a6

wrong "start" and "stride" default param, it should be undefined. Not…

ce5ad21

…e for future people

Merge branch 'dev' into dev-core-typescript

a3e846e

bigtimebuddy merged commit 1c4a90f into dev Jan 31, 2020

bigtimebuddy deleted the dev-core-typescript branch January 31, 2020 18:40

This was referenced Feb 1, 2020

refactor generateUniformsSync to use an exposed set of parsers #6369

Merged

Add stricter types for resource options and resource plugins #6373

Merged

Help Wanted: TypeScript Conversion Update #6377

Closed

Zyie mentioned this pull request Feb 11, 2020

add cube texture again #6405

Closed

3 tasks

Convert core to typescript #6340

Convert core to typescript #6340

Conversation

ivanpopelyshev commented Jan 17, 2020 • edited

bigtimebuddy commented Jan 17, 2020

bigtimebuddy commented Jan 19, 2020 • edited

ivanpopelyshev commented Jan 19, 2020 • edited

ivanpopelyshev commented Jan 19, 2020

bigtimebuddy commented Jan 19, 2020

Zyie commented Jan 19, 2020 • edited

ivanpopelyshev commented Jan 19, 2020

codecov-io commented Jan 22, 2020 • edited

Codecov Report

ivanpopelyshev commented Jan 22, 2020

Zyie commented Jan 22, 2020 • edited

bigtimebuddy commented Jan 22, 2020

EvidentlyCube commented Jan 22, 2020

Zyie left a comment

Choose a reason for hiding this comment

Zyie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivanpopelyshev Jan 23, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivanpopelyshev commented Jan 23, 2020

ivanpopelyshev commented Jan 23, 2020 • edited

ivanpopelyshev commented Jan 23, 2020

ivanpopelyshev commented Jan 23, 2020

ivanpopelyshev commented Jan 23, 2020

ivanpopelyshev commented Jan 23, 2020

ivanpopelyshev commented Jan 26, 2020 • edited

bigtimebuddy commented Jan 26, 2020

EvidentlyCube left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivanpopelyshev commented Jan 27, 2020

ivanpopelyshev commented Jan 27, 2020

bigtimebuddy commented Jan 29, 2020

ivanpopelyshev commented Jan 30, 2020

ivanpopelyshev commented Jan 30, 2020

bigtimebuddy commented Jan 30, 2020

ivanpopelyshev commented Jan 30, 2020

ivanpopelyshev commented Jan 31, 2020

ivanpopelyshev commented Jan 17, 2020 •

edited

bigtimebuddy commented Jan 19, 2020 •

edited

ivanpopelyshev commented Jan 19, 2020 •

edited

Zyie commented Jan 19, 2020 •

edited

codecov-io commented Jan 22, 2020 •

edited

Zyie commented Jan 22, 2020 •

edited

ivanpopelyshev Jan 23, 2020 •

edited

ivanpopelyshev commented Jan 23, 2020 •

edited

ivanpopelyshev commented Jan 26, 2020 •

edited