davidfig / pixi-viewport

A highly configurable viewport/2D camera designed to work with pixi.js
https://davidfig.github.io/pixi-viewport/
MIT License
1.04k stars 174 forks source link

Class 'Viewport' incorrectly extends base class 'Container' #63

Closed gsknbabu closed 6 years ago

gsknbabu commented 6 years ago

Hi David, I am getting following error with viewport. Your help is required. node_modules/pixi-viewport/@types/index.d.ts (176,15): Class 'Viewport' incorrectly extends base class 'Container'. Types of property 'on' are incompatible. Type '{ (event: ViewportEventType, fn: (viewport: Viewport) => void, context?: any): this; (event: View...' is not assignable to type '{ (event: "added" | "removed", fn: (displayObject: DisplayObject) => void, context?: any): this; ...'. Types of parameters 'event' and 'event' are incompatible. Type '"added" | "removed"' is not assignable to type 'ViewportEventType'. Type '"added"' is not assignable to type 'ViewportEventType'.

davidfig commented 6 years ago

Yeah, I've had a couple of reports on the @types error. I'm not that familiar with the definition file so it's difficult for me to debug and correct. I'll see if anyone can take a look. If not it might be time to dig into typescript to understand it once and for all.

dmecke commented 6 years ago

I worked around this issue with this:

let viewport = new Viewport({ ... }) as any;
app.stage.addChild(viewport);

My typescript skills are not too good, but as far as I understand this basically casts the viewport object to any, so typescript does no more complain about it's type issues in the @types file. This way you lose all of the type safety of course.

macropp commented 6 years ago

hi @dmecke, can you please provide Typescript version and how do you import Viewport?

Awkward to see all these issues since I'm able to use the type definitions in my project without problems.

For import I use:

import * as Viewport from "pixi-viewport";

image

The reason why typescript complains is obviously because I've stated that Viewport extends PIXI.Container to make things easier to maintain, but we don't extend it "correctly". This is the reason why I've added @ts-ignore markers where we "incorrectly" override on method.

Both TS 2.9.2 and 3.0.1 seem to be working fine in my project.

dmecke commented 6 years ago

I use TS 3.0.1 as well. Here is the minimal code, that reproduces the problem for me:

import * as PIXI from 'pixi.js';
import * as Viewport from 'pixi-viewport';

let app = new PIXI.Application();
document.body.appendChild(app.view);

let viewport = new Viewport();
app.stage.addChild(viewport);

The exact error message I get:

ERROR in C:\Users\[...]\src\app.ts
./src/app.ts
[tsl] ERROR in C:\Users\[...]\src\app.ts(8,20)
      TS2345: Argument of type 'Viewport' is not assignable to parameter of type 'DisplayObject'.
  Types of property 'on' are incompatible.
    Type '{ (event: ViewportEventType, fn: (viewport: Viewport) => void, context?: any): Viewport; (event: ViewportClickEventType, fn: (data: ViewportClickEventData) => void, context?: any): Viewport; (event: "wheel", fn: (data: ViewportWheelEventData) => void, context?: any): Viewport; }' is not assignable to type '(event: InteractionEventTypes, fn: (event: InteractionEvent) => void, context?: any) => DisplayObject'.
      Types of parameters 'event' and 'event' are incompatible.
        Type 'InteractionEventTypes' is not assignable to type 'ViewportEventType'.
          Type '"pointerdown"' is not assignable to type 'ViewportEventType'.

As you can see I am on a windows machine, I run it via webpack-dev-server. Really strange this does not happen in your environment, because it does not really feel to be an environment dependent problem. As you said, I think it's because of the on() definitions. Is there a specific reason you need to override this message from the PIXI.Container and changing the signature? Maybe using a different method for this would be better anyway?

macropp commented 6 years ago

Hey @dmecke, Thanks for providing the example. I'll look at that this evening. Regarding on(): the thing is that on method is part of pixi-viewport lib, because Viewport actually extends from PIXI.Container, but EventEmitter is reused to dispatch the events from Viewport. Viewport events are nothing like the original events that PIXI.Container emits, hence the issue. I'll try to look into options.

dmecke commented 6 years ago

Ah, I see. Maybe Viewport could use the EventEmitter via composition instead of inheritance, so it can keep it's public api clean and "PIXI-only".

macropp commented 6 years ago

Yeah, it's a nice suggestion. Wonder what @davidfig thinks about this? From the looks of it, quite some changes will need to be done to move to composition instead of inheritance.

davidfig commented 6 years ago

By using composition for the events, we lose PIXI.Container's events, unless I re-emit them, which is definitely not ideal.

Is there another way to work around this in the Typescript definition files?

Another alternative is to make the Viewport attach to a PIXI.Container. Then it can have its own event emitter. I played with that idea for a while, but eventually ended up inheriting because it made it easier to use. If there are other advantages, I'm open to revisiting this for a major version bump.