map virtual object to props

[vinnymac]

I was thinking about the API for this lib and I thought that the virtual prop seemed a bit odd to me. Wouldn't it make more sense if it worked like redux does with connect?

So in redux we might use mapStateToProps like in the example below.

const mapStateToProps = (state) => ({ todos: state.todos })
connect(mapStateToProps)(MyList)

We can simplify things by removing the virtual prop and adding a new option called mapVirtualToProps this would be a breaking change, but now users have total control over what props the HOC adds.

const mapVirtualToProps = (virtual) => ({ virtualStyle: virtual.style })
virtualList({ mapVirtualToProps })(MyList)

I would love to hear feedback on this idea.

[developerdizzle]

I definitely like this idea, although I'm going to need to think it through a bit more. Maybe it could be an optional second parameter to VirtualList which would help keep it backwards-compatible.

[vinnymac]

Yea, maybe we leave the virtual prop but document the new way? A kind of deprecation path similar to what other libs are doing would allow people to move to it over time. Let me know what you decide, I'll be available to make the necessary changes.

[developerdizzle]

Yeah, I think we can keep things backwards-compatible by providing a default function returning the same virtual object we have now.

I'm not sure if it makes more sense to have mapVirtualToProps as a propetry of options or a second argument to VirtualList, eg:

const MyVirtualList = VirtualList(options, mapVirtualToProps)(MyList);

I could go either way, although Redux implements it (as well as mapDispatchToProps) as its own argument, so maybe there's value in doing the same. What do you think?

Either way, I think it makes sense to have additional tests to cover when mapVirtualToProps is not provided (falling back to the default) and when it is provided.

[vinnymac]

I like the idea of using the second argument like they did. I set the default mapToVirtualProps as a default param, this should keep this from becoming a breaking change. Two tests were added for the cases you mentioned, let me know your thoughts.

[developerdizzle]

I've been thinking about this... and what if we give mapVirtualToProps the minimum arguments needed, in order to give more control to the developer. Although this also means some calculations are made here, which would likely have to be recreated if the dev was providing their own mapVirtualToProps

const defaultMapVirtualToProps = ({
  firstItemIndex,
  lastItemIndex,
  items,
  itemHeight,
}) => {
  const visibleItems = lastItemIndex > -1 ? items.slice(firstItemIndex, lastItemIndex + 1) : [];
  const height = items.length * itemHeight;
  const paddingTop = firstItemIndex * itemHeight;

  return {
    virtual: {
      items: visibleItems,
      style: {
        height,
        paddingTop,
      },
    },
  };
};

Or, alternatively, maybe we just make the incoming arguments a little more agnostic instead of assuming which style rules they'll be used for (I could see people wanting to use absolute positioning or maybe translateY or something else).

const defaultMapVirtualToProps = ({
  visibleItems,
  listHeight, // total height of the list
  listTop, // distance between the top of the list and the first visible item
}) => ({
  virtual: {
    items: visibleItems,
    style: {
      height: listHeight,
      paddingTop: listTop,
    },
  },
});

Maybe we include the other props here as well, likefirstItemIndex, itemHeight, etc. just in case the dev wants to use them.

I think it's important to get this right the first time, because if we change it later on, it will definitely be a breaking change.

[jordaaash]

I was wondering about this actually, have you implemented this with translateY at all?

[vinnymac]

@developerdizzle what if we take your calculations for visibleItems, and put that into a utility method getVisibleItems that we export? That way if others want to extend the behavior they can just use a helper to produce the same visibleItems. Over time if the logic changes it only changes in the helper.

The style calculations are simplistic enough that they shouldn't really need any kind of helper necessarily. If you felt strongly about those we could export an additional helper for those attributes as well.

At the end of the day, virtual could be separated into the two parts that compose it, props and state.

Below is an example of what I mean.

const defaultMapVirtualToProps = ({
  items, 
  itemHeight,
}, {
  firstItemIndex,
  lastItemIndex,
}) => {
  const visibleItems = getVisibleItems(firstItemIndex, lastItemIndex);
  const height = items.length * itemHeight;
  const paddingTop = firstItemIndex * itemHeight;

  return {
    virtual: {
      items: visibleItems,
      style: {
        height,
        paddingTop,
      },
    },
  };
};

// Further down in the code

render() {
  const props = {
    ...this.props,
    ...mapVirtualToProps(this.props, this.state)
  }

  return (<InnerComponent {...props} />);
};

This would allow a developer to not only use whatever helpers we deign important enough, but also allow them access to all the information we keep track of in the virtual list. Although moving towards this starts to make the name mapVirtualToProps sound a lot stranger.

[developerdizzle]

@jordansexton I did a long time ago, but I can't recall why I didn't end up using it.

@vinnymac I think I like this, although maybe we don't need getVisibleItems to be a separate function, as it's basically a one-liner. I'd vote we just keep that logic inside defaultMapVirtualToProps.

[developerdizzle]

Thanks for making those updates @vinnymac - I'm going to take a look at the tests this weekend and possibly push some changes.

[developerdizzle]

@vinnymac Just pushed up some changes - let me know what you think!

[vinnymac]

Moving the changes for defaultMapToVirtualProps to a separate file with its own tests makes sense. The code looks a lot cleaner this way. Also nice job on the documentation 👍

Hopefully this API is flexible enough for users, I can't think of anything else they might need from it right now.

[eryon]

It's arguments are should be Its arguments are

[developerdizzle]

Nice catch @eryon ! Grammar fail on my part..