Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

A simpler interface for react-move #54

Closed
dagda1 opened this issue Mar 5, 2019 · 12 comments
Closed

A simpler interface for react-move #54

dagda1 opened this issue Mar 5, 2019 · 12 comments

Comments

@dagda1
Copy link

dagda1 commented Mar 5, 2019

i've been having a playabout with react-move and I think we can simplify things down to the example on this typescript playground.

By letting the user supply their bits we can concentrate on the config.

Let me know what you think.
@sghall
Copy link
Owner

sghall commented Mar 6, 2019

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[]
  ...
}

Here's a sketch of what i think would work

What do you think?

@dagda1
Copy link
Author

dagda1 commented Mar 6, 2019

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

@dagda1
Copy link
Author

dagda1 commented Mar 6, 2019

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.

@sghall
Copy link
Owner

sghall commented Mar 6, 2019

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 } 
}

You also absolutely could have a mixed case. That's why I made a mixed case the example in the link I sent above. To make it clear that you have to handle those types of cases.

return {
      timing: { duration: 500 },
      top: point.y,
      left: point.x,
      opacity: 0,
      namespace1: {
        top: point.y,
        left: point.x,
        opacity: '0',
      }
    }

@dagda1
Copy link
Author

dagda1 commented Mar 6, 2019

@sghall great, these are useful examples.

@dagda1
Copy link
Author

dagda1 commented Mar 6, 2019

@sghall I think there is no sane way of defining the namespace and object to have the same type in typescript currently. I was well down the path of insanity there before I had a moment of sense.

If the user wants to use namespaces then they define that in their ts interface.

e.g.

No namespace:

export interface Point {
  x: number;
  y: number;
}

Namespace

export interface Coords {
  coords: Point;
}

Here is an updated version that still has widening problems which I can look at but is a whole lot easier

@sghall
Copy link
Owner

sghall commented Mar 6, 2019

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.

An idea: Just completely forget about real type safety by default in react-move:

All in this example

// 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?

@dagda1
Copy link
Author

dagda1 commented Mar 6, 2019 via email

@sghall
Copy link
Owner

sghall commented Mar 6, 2019

Yep. This has become unproductive. Sorry. Not going to deal with it.

@sghall sghall closed this as completed Mar 6, 2019
@dagda1

This comment has been minimized.

Sign in to view
@sghall

This comment has been minimized.

Sign in to view
@tannerlinsley
Copy link
Contributor

tannerlinsley commented Mar 6, 2019 •
edited

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.

Repository owner locked and limited conversation to collaborators Mar 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@dagda1 @sghall @tannerlinsley