Closed dagda1 closed 5 years ago
I think react-move is too flexible to have meaningful defined types. To me, the most valuable thing has been you suggesting a way to allow the user to define their own types. I don't know Typescript and had no idea that was even possible. That's what we should get out there, the built in types are going to fall short no matter what we do.
Here's what I think makes sense, let the user supply their own types and "override" the existing types. There's a bunch of edge cases here that will make the intersection between user defined and built in types not workable. I think we should provide some loose types that provide some basic type safety but then let the user override for their case.
Not sure on the syntax but something like this...
interface INodeGroupProps<D = HashMap, S = NodeState, C = Config> { // Provide a default, user overrides
start: (data: D, index: number) => S
enter?: (data: D, index: number) => C | C[]
...
}
What do you think?
I think we are nearly on the same page, I don't think we need the HashMap
indexer which basically cancels out all type safety and just let anything that is not Config
be in the State
type argument.
As far as namespaces go, is it always the case that you either don't have namespaces:
return {
top: point.y,
left: point.x,
opacity: 0,
timing: { duration: 500 }
}
Or you do have namespaces:
return {
namespace1: {
top: point.y,
left: point.x,
opacity: 0,
timing: { duration: 500 }
},
namespace2: {
top: point.y,
left: point.x,
opacity: 0,
timing: { duration: 500 }
}
}
But never:
return {
top: point.y,
left: point.x,
opacity: 0,
timing: { duration: 500 },
namespace1: {
top: point.y,
left: point.x,
opacity: '0',
timing: { duration: 500 }
}
}
If it is either you have namespaces or not then this covers that scenario
One deceptive thing about typescript and return types is you lose excess property checking on the return type of callbacks.
I came across this while trying to do this and wrote a blog post about it. This can give the illusion of type safety with extra properties not being type checked.
Great.
Just an FYI. On the namespaces, the examples you gave are NOT correct...
return {
namespace1: {
top: point.y,
left: point.x,
opacity: 0,
timing: { duration: 500 } // this is not possible
},
namespace2: {
top: point.y,
left: point.x,
opacity: 0,
timing: { duration: 500 } // this is not possible
}
}
Should be...
return {
namespace1: {
top: point.y,
left: point.x,
opacity: 0,
},
namespace2: {
top: point.y,
left: point.x,
opacity: 0,
},
timing: { duration: 500 }
}
return {
timing: { duration: 500 },
top: point.y,
left: point.x,
opacity: 0,
namespace1: {
top: point.y,
left: point.x,
opacity: '0',
}
}
@sghall great, these are useful examples.
Yeah, Typescript is just not made for this.
I think, what you sent is too opinionated to be the default and we seem to be going around in circles. To me, I keep saying there's a separate "State" and "Config" type and you keep sending back ideas where there's just the "State" type because it fits the narrow case you're thinking about or because Typescript can't deal with more dynamic types (really not sure). I think we can get around this with the solution below.
In any case, we have to allow namespaces and arrays of config objects out of the box (this also not covered in your examples, right?). People will come back here and complain if the stuff shown in the docs have typescript errors by default.
// No meaningful type safety, but no errors for standard usage with or without namespaces or sending an array of config objects
// Default types in react-move for NodeGroup
interface INodeGroupProps<T = any, State = any, Config = any> {
data: T[];
keyAccessor: (data: T, index: number) => string | number;
start: (data: T, index: number) => State
enter?: (data: T, index: number) => Config
update?: (data: T, index: number) => Config
leave?: (data: T, index: number) => Config
}
Move all of this to user land:
// ALL of this in user land
interface Config {
events?: Events
timing?: Timing
}
type MakeState<T> = {
[P in keyof T]: T[P] extends number | string ? T[P] | T[P][] : MakeState<Point>;
}
interface NodesState {
top: number;
left: number;
opacity: number;
}
type Point = { x: number, y: number }
const points: Point[] = [{ x: 0, y: 0 }, { x: 1, y: 2 }, { x: 2, y: 3 }]
interface Coords {
coords: Point;
}
type MyConfig = MakeState<Coords> & Partial<Config>
const nodesWithNamespaces: INodeGroupProps<Point, Coords, MyConfig> = {
data: points,
keyAccessor: d => {
return d.x
},
start: point => {
return {
coords: {
x: point.x,
y: point.y,
},
opacity: 0,
}
},
enter: point => {
return {
coords: {
x: [point.x],
y: [point.y],
},
timing: { duration: 1000, delay: 500 }
}
},
}
Is there anything that couldn't be made reasonably type safe doing it this way?
the last example i sent fits all the examples in the readme and we are going round in circles with your replies in capital letters of how i got the examples wrong when it is your project.
my example does cover namespaces but for some reason you don’t see this.
i’m trying to help with limited knowledge of the project but i can do without the impatience
On Wed, 6 Mar 2019 at 19:00, Steve Hall notifications@github.com wrote:
Yeah, Typescript is just not made for this.
I think, what you sent is too opinionated to be the default and we seem to be going around in circles. To me, I keep saying there's a separate "State" and "Config" type and you keep sending back ideas where there's just the "State" type because it fits the narrow case you're thinking about or because Typescript can't deal with more dynamic types (really not sure). I think we can get around this with the solution below.
// No meaningful type safety, but no errors for standard usage with or without namespaces or sending an array of config objects
// Default types in react-move for NodeGroup
interface INodeGroupProps<T = any, State = any, Config = any> { data: T[]; keyAccessor: (data: T, index: number) => string | number; start: (data: T, index: number) => State enter?: (data: T, index: number) => Config update?: (data: T, index: number) => Config leave?: (data: T, index: number) => Config }
Move all of this to user land:
// ALL of this in user land interface Config { events?: Events timing?: Timing }
type MakeState
= { [P in keyof T]: T[P] extends number | string ? T[P] | T[P][] : MakeState ; } interface NodesState { top: number; left: number; opacity: number; }
type Point = { x: number, y: number }
const points: Point[] = [{ x: 0, y: 0 }, { x: 1, y: 2 }, { x: 2, y: 3 }]
interface Coords { coords: Point; }
type MyConfig = MakeState
& Partial const nodesWithNamespaces: INodeGroupProps<Point, Coords, MyConfig> = { data: points, keyAccessor: d => { return d.x }, start: point => { return { coords: { x: point.x, y: point.y, }, opacity: 0, } }, enter: point => { return { coords: { x: [point.x], y: [point.y], }, timing: { duration: 1000, delay: 500 } } }, }
Is there anything that couldn't be made reasonably type safe doing it this way?
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/react-tools/react-move/issues/54#issuecomment-470232367, or mute the thread https://github.com/notifications/unsubscribe-auth/AAHOONAi_3FGieMGvSJRdlwlQd0Inyddks5vUBBKgaJpZM4beFkK .
-- Cheers
Paul Cowan
Cutting-Edge Solutions (Scotland)
blog: http://thesoftwaresimpleton.com/ website: https://cutting.scot/
Yep. This has become unproductive. Sorry. Not going to deal with it.
you are an idiot
On Wed, 6 Mar 2019 at 19:17, Steve Hall notifications@github.com wrote:
Yep. This has become unproductive. Sorry. Not going to deal with it.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/react-tools/react-move/issues/54#issuecomment-470238749, or mute the thread https://github.com/notifications/unsubscribe-auth/AAHOOJljRU923KKb3lCAuf9pBc8Zlv5Zks5vUBRCgaJpZM4beFkK .
-- Cheers
Paul Cowan
Cutting-Edge Solutions (Scotland)
blog: http://thesoftwaresimpleton.com/ website: https://cutting.scot/
Whoa. Who the fuck do you think you're talking to?
For posterity, @sghalls recommendations work just fine. It shouldn't be the expectation that users can rely 100% on the types of a library when the structure of those types can dynamically change due to user code. It's reasonable for React Move to provide types for the interfaces it can control and unfortunately that's the most reliability we can offer. Once an isolated piece of code (like this library) calls into user-land code, user-land types will be necessary.
I think this is a great testament to why TS provides value while also creating new problems.
i've been having a playabout with react-move and I think we can simplify things down to the example on [this](http://www.typescriptlang.org/play/index.html#src=%0D%0Aexport%20type%20EasingFunction%20%3D%20(t%3A%20number)%20%3D%3E%20number%0D%0Aexport%20type%20Interpolator%20%3D%20(t%3A%20number)%20%3D%3E%20void%0D%0Aexport%20type%20Tween%20%3D%20()%20%3D%3E%20((t%3A%20number)%20%3D%3E%20void)%20%7C%20null%0D%0A%0D%0Aexport%20interface%20Timer%20%7B%0D%0A%20%20%20%20restart(callbackFn%3A%20(elapsed%3A%20number)%20%3D%3E%20void%2C%20delay%3F%3A%20number%2C%20time%3F%3A%20number)%3A%20void%3B%0D%0A%20%20%20%20stop()%3A%20void%3B%0D%0A%7D%0D%0A%0D%0Aexport%20interface%20Timing%20%7B%0D%0A%20%20time%3F%3A%20number%0D%0A%20%20delay%3F%3A%20number%0D%0A%20%20duration%3F%3A%20number%0D%0A%20%20ease%3F%3A%20EasingFunction%0D%0A%7D%0D%0A%0D%0Aexport%20interface%20Eventable%20%7B%0D%0A%20%20%5Bkey%3A%20string%5D%3A%20()%20%3D%3E%20void%0D%0A%20%20start%3F%3A%20()%20%3D%3E%20void%0D%0A%20%20interrupt%3F%3A%20()%20%3D%3E%20void%0D%0A%20%20end%3F%3A%20()%20%3D%3E%20void%0D%0A%7D%0D%0A%0D%0A%0D%0Aexport%20interface%20Transition%20%7B%0D%0A%20%20status%3A%20number%0D%0A%20%20timing%3A%20Timing%0D%0A%20%20timer%3F%3A%20Timer%0D%0A%20%20tweens%3A%20Array%3C()%20%3D%3E%20Interpolator%3E%0D%0A%20%20events%3A%20Eventable%0D%0A%20%20stateKey%3A%20string%0D%0A%7D%0D%0A%0D%0A%0D%0Aexport%20interface%20Config%20%7B%0D%0A%20%20events%3F%3A%20Eventable%0D%0A%20%20timing%3F%3A%20Timing%0D%0A%7D%0D%0A%0D%0Aexport%20type%20AddArrayLike%3CT%3E%20%3D%20%7B%0D%0A%20%20%5BP%20in%20keyof%20T%5D%3A%20T%5BP%5D%20%7C%20T%5BP%5D%5B%5D%0D%0A%7D%0D%0A%0D%0Aexport%20interface%20INodeGroupProps%3CT%20%3D%20unknown%5B%5D%2C%20State%20%3D%20%7B%7D%3E%20%7B%0D%0A%20%20data%3A%20T%5B%5D%3B%0D%0A%20%20keyAccessor%3A%20(data%3A%20T%2C%20index%3A%20number)%20%3D%3E%20string%20%7C%20number%3B%0D%0A%20%20start%3A%20(data%3A%20T%2C%20index%3A%20number)%20%3D%3E%20%20State%20%26%20Config%3B%0D%0A%20%20enter%3F%3A%20(data%3A%20T%2C%20index%3A%20number)%20%3D%3E%20AddArrayLike%3CConfig%3E%20%26%20AddArrayLike%3CState%3E%3B%0D%0A%20%20update%3F%3A%20(data%3A%20T%2C%20index%3A%20number)%20%3D%3E%20%20AddArrayLike%3CConfig%3E%20%26%20AddArrayLike%3CState%3E%3B%0D%0A%20%20leave%3F%3A%20(data%3A%20T%2C%20index%3A%20number)%20%3D%3E%20AddArrayLike%3CConfig%3E%20%26%20AddArrayLike%3CState%3E%3B%0D%0A%7D%0D%0A%0D%0Aexport%20interface%20NodesState%20%7B%0D%0A%20%20top%3A%20number%3B%0D%0A%20%20left%3A%20number%3B%0D%0A%20%20opacity%3A%20number%3B%0D%0A%7D%0D%0A%0D%0Aexport%20type%20Point%20%3D%20%7B%20x%3A%20number%2C%20y%3A%20number%20%7D%0D%0A%0D%0Aconst%20points%3A%20Point%5B%5D%20%3D%20%5B%7B%20x%3A%200%2C%20y%3A%200%20%7D%2C%20%7B%20x%3A%201%2C%20y%3A%202%20%7D%2C%20%7B%20x%3A%202%2C%20y%3A%203%20%7D%5D%0D%0A%0D%0Aconst%20Nodes%3A%20INodeGroupProps%3CPoint%2C%20NodesState%3E%20%3D%20%7B%0D%0A%20%20data%3A%20points%2C%0D%0A%20%20keyAccessor%3A%20d%20%3D%3E%20%7B%0D%0A%20%20%20%20return%20d.x%0D%0A%20%20%7D%2C%0D%0A%20%20start%3A%20point%20%3D%3E%20%7B%0D%0A%20%20%20%20return%20%7B%0D%0A%20%20%20%20%20%20top%3A%20point.y%2C%0D%0A%20%20%20%20%20%20left%3A%20point.x%2C%0D%0A%20%20%20%20%20%20opacity%3A%200%2C%0D%0A%20%20%20%20%20%20timing%3A%20%7Bduration%3A%20500%7D%0D%0A%20%20%20%20%7D%0D%0A%20%20%7D%2C%0D%0A%20%20enter%3A%20point%20%3D%3E%20%7B%0D%0A%20%20%20%20return%20%7B%0D%0A%20%20%20%20%20%20top%3A%20%5Bpoint.y%5D%2C%0D%0A%20%20%20%20%20%20left%3A%20%5Bpoint.x%5D%2C%0D%0A%20%20%20%20%20%20opacity%3A%20%5B0%5D%2C%0D%0A%20%20%20%20%7D%0D%0A%20%20%7D%2C%0D%0A%20%20update%3A%20point%20%3D%3E%20%7B%0D%0A%20%20%20%20return%20%7B%0D%0A%20%20%20%20%20%20top%3A%20%5Bpoint.y%5D%2C%0D%0A%20%20%20%20%20%20left%3A%20%5Bpoint.x%5D%2C%0D%0A%20%20%20%20%20%20opacity%3A%20%5B0%5D%2C%0D%0A%20%20%20%20%7D%0D%0A%20%20%7D%2C%0D%0A%20%20leave%3A%20point%20%3D%3E%20%7B%0D%0A%20%20%20%20return%20%7B%0D%0A%20%20%20%20%20%20top%3A%20%5Bpoint.y%5D%2C%0D%0A%20%20%20%20%20%20left%3A%20%5Bpoint.x%5D%2C%0D%0A%20%20%20%20%20%20%20%20opacity%3A%20%5B0%5D%2C%0D%0A%20%20%20%20%20%20%20%20events%3A%20%7B%0D%0A%20%20%20%20%20%20%20%20%20%20%20%20start%3A%20()%20%3D%3E%20%7B%0D%0A%20%20%20%20%20%20%20%20%20%20%20%20%20%20console.log('do%20something')%0D%0A%20%20%20%20%20%20%20%20%20%20%7D%0D%0A%20%20%20%20%20%20%7D%0D%0A%20%20%20%20%7D%0D%0A%20%20%7D%2C%0D%0A%7D) typescript playground.
By letting the user supply their bits we can concentrate on the config.
Let me know what you think.