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

Converts display package to typescript #6261

Merged
merged 11 commits into from
Jan 17, 2020
Merged

Conversation

Zyie
Copy link
Member

@Zyie Zyie commented Dec 4, 2019

UPDATE!!

Now that #6311 has fixed the build issues this PR is ready to have its time to shine.

This PR converts the display package to typescript.

CHANGES:

  • DisplayObject is now an abstract class.
  • tempDisplayObjectParent is now a new class called TemporaryDisplayObject. This simply extends DisplayObject as you cannot instantiate an abstract class.
  • Inside Renderer.js ive added the systems used in display to be this.SYSTEM instead of being dynamically added. @ivanpopelyshev I've added a TODO as a note that this was hacked in since you are converting core.
  • Removed this.rect from Bounds as it was not being used.
  • Made _parentID public in Transform

I've also seen that #6304 and ##6306 has made some changes to display. Not sure whats going to be the best way to navigate this.

@ivanpopelyshev
Copy link
Collaborator

ivanpopelyshev commented Dec 4, 2019

Yes, DisplayObject is meant to be abstract, to separate properties of this element, and properties that depend on children.

There're no classes that are DisplayObject but not Container. Originally in Flash there were SimpleButton's and TextFields, and it was hell.

I think rule has to be "use DisplayObject if you don't care about children of element".

Line 469 : type is DisplayObject. MaskData was introduced by me in 5.2.0, its for users who need more control on masks.

@EvidentlyCube
Copy link
Contributor

EvidentlyCube commented Dec 4, 2019

I've got similar issue as the first one you mentioned but with @pixi/constants, so I am curious how to handle it.

Edit: And it appears a solution is to add the declare module to global.d.ts

@ivanpopelyshev
Copy link
Collaborator

OK, I've started @pixi/core conversion, renamed files to TS, started converting texture/resources/Resource.ts and stumbled across Runner import. I don't feel like adding stuff to global.d.ts. It was fine in first modules, but here we'll have to import too many things, especially from @pixi/math module.

I think its time to figure out how to generate d.ts files for a few converted modules to use instead global.d.ts

@EvidentlyCube
Copy link
Contributor

@ivanpopelyshev A temporary solution might be to add the following line to package.json of each module that's already been converted to TypeScript:

  "types": "src/index.ts",

I'll tag you in a conversation I had on slack.

@englercj
Copy link
Member

Ideally Runner provides typescript types so the import just works.

global.d.ts Outdated Show resolved Hide resolved
@Zyie Zyie marked this pull request as ready for review January 7, 2020 20:34
@Zyie Zyie changed the title Start of converting display package to typescript Converts display package to typescript Jan 7, 2020
@codecov-io
Copy link

codecov-io commented Jan 8, 2020

Codecov Report

Merging #6261 into dev will increase coverage by 0.41%.
The diff coverage is 80%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev    #6261      +/-   ##
==========================================
+ Coverage   76.74%   77.15%   +0.41%     
==========================================
  Files         182      182              
  Lines        9497     9530      +33     
==========================================
+ Hits         7288     7353      +65     
+ Misses       2209     2177      -32
Impacted Files Coverage Δ
packages/prepare/src/Prepare.js 96.42% <ø> (ø) ⬆️
packages/core/src/shader/ShaderSystem.js 93.58% <ø> (ø) ⬆️
packages/prepare/src/TimeLimiter.js 100% <ø> (ø) ⬆️
packages/prepare/src/CountLimiter.js 100% <ø> (ø) ⬆️
packages/core/src/Renderer.js 89.61% <ø> (ø) ⬆️
bundles/pixi.js/test/index.js 100% <ø> (ø) ⬆️
bundles/pixi.js-legacy/test/index.js 100% <ø> (ø) ⬆️
...kages/canvas/canvas-renderer/src/CanvasRenderer.js 85.84% <ø> (ø) ⬆️
packages/prepare/src/BasePrepare.js 73.43% <ø> (ø) ⬆️
packages/text/src/Text.js 66.51% <0%> (-0.3%) ⬇️
... and 13 more

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 b014584...49c0329. Read the comment docs.

@laino
Copy link
Contributor

laino commented Jan 8, 2020

I've also seen that #6304 and ##6306 has made some changes to display. Not sure whats going to be the best way to navigate this.

Don't worry about them. One is a simple bugfix (a bug that would be caught by TypeScript btw.), and the other is just something I wanted to get some input for before making it a serious PR (I'm unsure what implications my changes might have).

Both are pretty easy to rebase on/rebase on this. I can do the latter.

@bigtimebuddy bigtimebuddy added this to the v5.3.0 milestone Jan 10, 2020
Defines a type for the cursor property of IHitArea in global.d.ts
packages/display/src/Bounds.ts Outdated Show resolved Hide resolved
packages/display/src/Container.ts Show resolved Hide resolved
packages/display/src/Container.ts Outdated Show resolved Hide resolved
Adds initialization in constructor for Bounds
Makes containerUpdateTransform publicly accessible
@ivanpopelyshev
Copy link
Collaborator

All resolved, good job.

@bigtimebuddy
Copy link
Member

@Zyie could you resolve these conflicts? I think we're ready to merge.

@ivanpopelyshev
Copy link
Collaborator

I see problems with _updateID, gonna fix them for you, its my area of expertise anyway. Maybe I'll add a test.

@ivanpopelyshev ivanpopelyshev mentioned this pull request Jan 16, 2020
@Zyie
Copy link
Member Author

Zyie commented Jan 16, 2020

@Zyie could you resolve these conflicts? I think we're ready to merge.

All sorted

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.

None yet

8 participants