search

ianstormtaylor / react-values

A set of tiny React components for handling state with render props.
https://git.io/react-values
MIT License
1.02k stars 39 forks source link

TypeScript definitions #25

Closed theKashey closed 5 years ago

theKashey commented 6 years ago

Source issue: #8

Adding TS defs for all the components, and partial interface smoke tests.

As I understand - all proxyified functions are returning nothing, and only affecting the stored value.

theKashey commented 6 years ago

My bad, I did not check build status. Now it's green.

ianstormtaylor commented 5 years ago

Hey @theKashey, I'm sorry for the delay here. I wanted to get the "connected" value concept into the codebase before I merge this, since I'm not familiar with TypeScript and wasn't sure I'd be able to write the types for it properly. Is there any way you could update it to incorporate that concept from 0.3.0? Thank you so much!

theKashey commented 5 years ago

@ianstormtaylor - any documentation?

theKashey commented 5 years ago

Found it. Should be straightforward. Will update typings in after hours.

Regardless - why did you create this pattern? It's quite useful, but could be dangerous.

Personally, I am just pushing props from "Controller" to ContextAPI and then pulling data back. Ie Gearbox.

theKashey commented 5 years ago

Done

ianstormtaylor commented 5 years ago

@theKashey dangerous, how so? It's hard for me to understand exactly what Gearbox is doing from its Readme, but that might be because I just woke up 😄

Thank you!

theKashey commented 5 years ago

Gearbox is like react-adopt - small tool for context, or renderProp composition. Then it pushes result into the Context, giving you ability to pop result somewhere down the tree. Same "connect", but in a vertical mode without "horizontal" side effect. Connect is a singleton, and could make testing just impossible, as long one test will soil another.

PS: I've got a build flake. Could you trigger rebuild?

ianstormtaylor commented 5 years ago

Thanks @theKashey!

ianstormtaylor commented 5 years ago

Hey @theKashey, I've just been playing with this, trying to work with updating the definitions and tests myself to see how complex the maintenance would be, and I've got to say, since this library uses generics so heavily and I'm such a novice at typings, the TypeScript logic is extremely hard to wrap my head around.

Even trying to refactor the existing typings to get them named how I'd like the exports to be is extremely confusing for me, and refactoring the tests to be a bit simpler to write/read. I've been running into strange errors that are hard to decipher, and realizing that maintaining these across versions is going to be out of my depth.

For that reason, I'm going to revert the change adding the types. I'm really sorry to have wasted your time here. I'd love if you could contribute these to @types instead. I just realized that the React typings themselves live there already, and since this library's API contract is fairly stable it shouldn't be too hard to manage over time for those who are familiar with TypeScript in DefinitelyTyped.

I'm sorry for wasting your time here, I hope you understand. Thanks again.

theKashey commented 5 years ago

No worries. But please invest some time in "typed" javascript - one day it will save you.