michalochman / react-pixi-fiber

Write PixiJS applications using React declarative style.
MIT License
868 stars 94 forks source link

Support typescript 2.9.x #58

Closed seethroughdev closed 6 years ago

seethroughdev commented 6 years ago

Description

It appears that upgrading to v2.9 highlights a type issue:

(18,57): Type 'keyof T' does not satisfy the constraint 'string'.
  Type 'string | number | symbol' is not assignable to type 'string'.
    Type 'number' is not assignable to type 'string'.

It looks like Diff is expecting a string, but Omit is not specifying that keyof T should be a string.

also, incredible job with the index.d.ts. there's a lot to learn from here!

Steps to reproduce

  1. upgrade to typescript 2.9.x

Additional info

michalochman commented 6 years ago

index.d.ts is mostly work of @jmfirth, with further changes by @HibikineKage and @Fabiantjoeaon.

I never considered using typescript, so I'll not be able to fix any issues until I learn how the typing works under the hood.

If you know how to fix it that would not break typescript versions prior to 2.9 you are very welcome to open PR.

nervestaple commented 6 years ago

@seethroughtrees I saw this issue in another library and submitted a PR. it was pretty easy and i believe it should be backwards compatible as it's more permissive, not less? but haven't tested it.

https://github.com/router5/router5/pull/304

Epskampie commented 6 years ago

This problem is because typescript now allows string number and Symbol for keyof types instead of just string, see for more info: https://blogs.msdn.microsoft.com/typescript/2018/05/31/announcing-typescript-2-9/#breaking-changes

The change that @nervestaple suggests should work.

Epskampie commented 6 years ago

@seethroughtrees : i cannot seem to reproduce this using typescript 2.9.1 and the other versions of software you suggested. Can you share a small snippet or repo that shows the issue?

Here is my small test that seems to be working if you run npm i && npm run watch: https://github.com/Epskampie/test-pixi-fiber

seethroughdev commented 6 years ago

Sorry for the delay, i've been traveling. Thanks everyone for the input.

That does appear to be the exact change that is breaking things. Thanks @Epskampie. I don't have a public repo to share, but can try and put something together soon.

I originally did a very similar change to what you posted @nervestaple . However, it wasn't enough as some other functions depend on it as well.

I'm still traveling for the next couple of days. but if there isn't a PR by the time I get back, I'll dig in and try to find a good fix.

seethroughdev commented 6 years ago

and you're right @Epskampie . I ran your test repo, and it does compile fine. the project I used this on, is a create-react-app. will have to compare the compile settings later. but definitely what you made works as expected for me as well.

seethroughdev commented 6 years ago

Ok, I created the same repo as @Epskampie with https://github.com/wmonk/create-react-app-typescript. and am seeing the originally posted error.

https://github.com/seethroughtrees/test-pixi-fiber

Will try and dig into some solutions, but still open to hearing any other thoughts.

seethroughdev commented 6 years ago

I should also note, the above is temporarily fixed with by adding the tsconfig option "keyofStringsOnly": true which obviously isn't a longterm fix.

https://blogs.msdn.microsoft.com/typescript/2018/05/31/announcing-typescript-2-9/#breaking-changes

seethroughdev commented 6 years ago

Submitted a PR that appears to work as expected, but would appreciate a review from any of the maintainers familiar with typescript to confirm.

https://github.com/michalochman/react-pixi-fiber/pull/59

hope it helps.

michalochman commented 6 years ago

Closed by #59