Skip to content
This repository has been archived by the owner on Dec 31, 2020. It is now read-only.

TypeScript, MobX-React, and RefObjects #619

Closed
wants to merge 10 commits into from

Conversation

42shadow42
Copy link

Fixed issue where inject decorated classes were not compatible with strongly typed RefObjects

42shadow42 and others added 8 commits December 5, 2018 09:04
As appropriate for higher order components, injector should now forwardRefs
…en't fixed because the higher order component is returned instead of the Injector and properties are lost
…nce property is no longer possible or necessary the component can be accessed directly through the RefObject API
@42shadow42
Copy link
Author

Fixes #616

This is missing unit tests currently and should be added, I'm not really an expert here so I wouldn't know how to write them myself.

This should be treated as a breaking change, as it affects the usage of wrappedInstance on RefObjects, the migration path is to remove the .wrappedInstance and use the item directly.

@mweststrate
Copy link
Member

@42shadow42 I need a little clarification. Is the goal to fix typings, or do you want to enable refForwarding on inject? Which of those two would be according to you the correct fix?

In either case, could you demonstrate what you are fixing in a small reproduction (codesandbox, unit test or otherwise), so that the solution can be verified?

@42shadow42
Copy link
Author

42shadow42 commented Dec 12, 2018

@mweststrate The goal is to fix typings, the proposed solution is refForwarding.

Here is my reproduction:
mobx-injector.zip

If you look at parent-component.tsx you will see that typescript enforces typings of child and child2 variables, but the actual runtime types are inject wrappedInstances so while the typescript compiles runtime fails, the proposed fix of refForwarding makes the compile time type match the runtime type.

Edit: Fixed reproduction so it compiles

@mweststrate mweststrate mentioned this pull request Feb 11, 2019
15 tasks
mweststrate added a commit that referenced this pull request Feb 11, 2019
@mweststrate
Copy link
Member

Thanks for the PR! I took the important changes, but refactored and cleaned up the as it got a bit messy in #644. Also added / updated tests.

So closing this one in favor of that one, but thanks for pointing out the solution and will put your name on the contributors list!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants