Allow getter hooks to reuse the previous props

[kizu]

I'm digging into the getter hooks, like the getCellProps etc. — what I'm wondering is if it is somehow possible right now to access the previous values of the props, in order to manually merge/reuse them?

Example: I want to access the current colSpan, and add some styles based on it or modify it. But from what I've seen, this is not possible from the hooks. If I'm not missing something, then I can see this could be added in one of the following ways:

1. Getter hooks could have the third argument after the Cell and TableInstance, which would contain the currently accumulated props.

2. Alternatively, the currently accumulated props could be probably stored inside a Cell?

Currently, I can see that the only way of doing what I want is outside of a plugin-hook, by using the cell.getCellProps() during the Table generation and passing its values to my other function. But I feel that having a way to access the props from the previous hooks could be very powerful and could allow to create quite flexible plugin-hooks :)

[tannerlinsley]

What you're referring to I did have briefly in this version. It was essentially a reducer, much like the applyHooks utility, and it would give you the previous props and allow you to return the new ones (with meta and instance as 2nd and 3rd params to the reducer function).

One issue with this was just the shell-shock that people got when they used it for the first time in their renderer

<div {...reduceCellProps(prev => ({...prev, newProp: newValue })}>

Which is why I moved to the simplified form later on. I can see your point that you want access to the accumulated values though. Currently inside of the system, I use mergeProps to effectively do this:

const accumulatedProps = applyPropHooks(getHeaderPropsHooks(), column, instanceRef.current)

// You can use/infer from `accumulatedProps` here

const newProps = mergeProps(
        {
          key: ['header', column.id].join('_'),
          colSpan: column.totalVisibleHeaderCount,
        },
        accumulatedProps,
      )

So yeah, it's just a different flavor like you said. Now that being said, I kinda do like the idea of going back to a reducer pattern, since that would technically allow you to set defaults as a plugin dev:

hooks.getRowProps.push((props, row, instance) => ({
  defualtProp: defaultValue,
  ...props,
  newProps: newValue
}))

But really it all comes down to that getCellProps api. It's very convenient and also very safe for most users to interact with and not shoot themselves in the foot. Once you move to a reduce in that space, it gets messy.

WHAT IF... the hook api used a reducer, but the final result was still the getThingProps function? I personally like that idea a lot. It would, as you mentioned, add a lot of power to the hooks api for props.

[tannerlinsley]

AAANNNDDD... How about we overload the args to getThingProps with the ability to pass a function. Then you get the best of both worlds and a graceful upgrade. So you get the simple approach:

<div {...getCellProps({ customProp: true })}>

And you get the powerful reducer style:

<div {...getCellProps(prev => ({ defaultProp: defaultValue, ...prev, customProp: true }) )}>

[kizu]

Ok, your second comment made me think about what if we would allow the hook to return either the props object (what we have now), or a reducer:

hooks.getRowProps.push((row, instance) => prev => ({
  defualtProp: defaultValue,
  ...prev,
  newProps: newValue
}))

This would make the default the same as it is now, but then there would be an option to have a reducer. This should make it so it would be hard to shoot yourself in the foot I think? And this would make it consistent with ...getCellProps() in the renderer that could also have either an object (like now) or a reducer (which you propose in the second comment)?

[tannerlinsley]

I have a pretty decent solution that will fit all of these use cases. I'm currently working up a PR. Hang tight.

[tannerlinsley]

Looking good so far. I'll push and release in a few minutes :)

[tannerlinsley]

Alright, this might be a bit much, but you need to take a look :)

4a33110

[tannerlinsley]

The most important parts are how the prop getters work:

// Auto extend with smart-merging styles and classNames
hooks.getThingProps.push({
  newProp: true,
})

// Multi auto extend with smart-merging styles and classNames
hooks.getThingProps.push([{
  style: {...},
  className: 'one'
}, {
  style: {...}, 
  className: 'two'
}])

// Functional replacement with both options above:
hooks.getThingProps.push((props, instance, row) => [
  props,
  {
    style: {...}, 
    className: 'two'
  }
])

// This will ditch old props
hooks.getThingProps.push((props, instance, row) => [
  // props,
  {
    style: {...}, 
    className: 'two'
  }
])

// This will do defaults
hooks.getThingProps.push((props, instance, row) => [
  {
    style: {...}, 
    className: 'two'
  }
  props,
])

// This will replace all props without smart merging
hooks.getThingProps.push((props, instance, row) => [
  {
    ...props,
    style: {...}, 
    className: 'two'
  }
])

More importantly, all of these options are also supported via the actual resulting prop getter function on the instance. So you should never need to use mergeProps either (which is also why it's deprecated now 😉)

[tannerlinsley]

It's non-plugin user-facing flexibility is shown off pretty well here: https://github.com/react-tools/react-table/blob/4a3311035d036c97f764f18faa510ecedb43402c/examples/data-driven-classes-and-styles/src/App.js#L76

[tannerlinsley]

I feel like that should be more than sufficient :) If you have more issues/questions, let me know.

[kizu]

Great!

hooks.getThingProps.push({
  newProp: true,
})

Ohh, this is cool and very nice for the simple cases :)

Also, looking at the commit, makePropGetter is something I wanted to ask for as well, would try it as well for my stuff!

[kizu]

@tannerlinsley The only thing I have noticed (but didn't investigate deeply yet) is that the getters are a bit slower now, you might want to compare them for bigger tables with a few hooks added before and after the API change. I'd also want to check this out, but no ETA for this as I'm doing some other stuff right now.

[tannerlinsley]

Yeah they do have a bit more overhead to support all of the things. Thus far I haven’t seen any slowdown with the existing examples except for column resizing but I believe it’s unrelated.

[kizu]

In rc.10 there was a noticeable improvement for me, great work!