ArrowKeyStepper: setState(...) in componentWillUpdate

[alexandro81]

Hi,
not sure if this should be considered an issue, but I thought it would not hurt to talk about it.
In the ArrowKeyStepper component, into the componentWillUpdate React lifecycle method, there is a call to setState(...).
I am not entirely sure that's correct. (https://facebook.github.io/react/docs/react-component.html#componentwillupdate).
Isn't it a best practice to use componentWillReceiveProps instead?

Also, I see that the state is updated if the new set of props differ from the old set of props.
But this way, if the component updates its state as a result of key down events and then it receives a new pair of values for scrollToRow and scrollToIndex from a parent component (say, for example, as the result of clicking on one of the items of the wrapped List or Grid), those values will not end up in the state, and the component will not be able to reflect those in its render method.
(if not clear, I'll try to come up with a small demo).

Thoughts?

[bvaughn]

You're right in pointing out that the component should not be calling setState during the componentWillUpdate lifecycle. This looks like an oversight on my part. Fixed via 06c7e50.

I'm not sure I follow on your second paragraph. If props change, I assume the intent is for the prop to override the state. So what you're describing sounds like what I would expect to be desired?

[alexandro81]

Hey Brian,
hopefully an example will clarify.
The problem I have is that I want to be able to focus on a item of my Grid component (child of ArrowKeyStepper) with both a mouse click on the item and keyboard navigation.

Here's the working example: http://plnkr.co/edit/jgTqAr1CTZSil5Un5xKZ?p=preview

I'm referencing the latest source of ArrowKeyStepper (with your fix), with this minor change:

componentWillReceiveProps (nextProps) {
const { scrollToColumn, scrollToRow } = nextProps

const {
  scrollToColumn: prevScrollToColumn,
  scrollToRow: prevScrollToRow
} = this.state

if (
  prevScrollToColumn !== scrollToColumn &&
  prevScrollToRow !== scrollToRow
) {
  this.setState({
    scrollToColumn,
    scrollToRow
  })
} else if (prevScrollToColumn !== scrollToColumn) {
  this.setState({ scrollToColumn })
} else if (prevScrollToRow !== scrollToRow) {
  this.setState({ scrollToRow })
}

}

The difference is that I'm reading prevScrollToColumn and prevScrollToRow from the current state.

Here's why:
keyboard navigation updates those values in the state.
I have an hierarchy like this: GridParent -> ArrowKeyStepper -> VirtualGrid
has in its state a pair of values (rowIndex, colIndex) that represent the "coordinates" of the last focused item with a mouse click.
If in componentWillReceiveProps of ArrowKeyStepper I didn't compare the current state (set by keyboard navigation) with the new props (passed by GridParent), I wouldn't be able to update the internal state of the component.

It'd be great if you could take a look and tell me if you got my point and if you think that's a correct approach (that would eventually require a code change) or not.

Thx!

[bvaughn]

The problem with using props for things like this is that it's often ambiguous what should happen. I was intentional about comparing previous props to current props (rather than state) because I didn't want a change in an unrelated prop to override local state. For example...

// First render
// User is initially scrolled to row 50 by default
<ArrowKeyStepper
  columnCount={100}
  rowCount={100}
  scrollToRow={50}
/>

// User uses arrow keys to move down to row 100 at which point more rows load
// And we re-render with an updated row count (and no other props changes)
<ArrowKeyStepper
  columnCount={100}
  rowCount={200}
  scrollToRow={50}
/>

In the above example, if we compared props to state then the arrow stepper would be reset back to row 50- which is not what we'd want. Arguably we shouldn't pass a prop through at all in this second render but... I think people will do this anyway in many cases.

Maybe the thing to do here is what I've done with other components and just add a public method for explicitly overriding this property (eg scrollToRow allows users to explicitly set/reset the scrollToIndex prop). How would you feel about that?

[alexandro81]

So you're basically saying to add a method in ArrowKeyStepper like this ?

setScrollPosition(scrollToRow, scrollToColumn) { this.setState({ scrollToRow, scrollToColumn }) }

Not sure how I could access an instance of the element in order to call this method.
Were you thinking to expose such a method in as an additional property in the children(...) function?
Do you have an example I can look at for another component, whereby you're overriding its internal state in a way that is closer to what I need to do?

[bvaughn]

Not sure how I could access an instance of the element in order to call this method.

With a ref 😄

class MyComponent extends Component {
  render () {
    return (
      <ArrowKeyStepper
        {...this.props}
        ref={this._setRef}
      />
    )
  }

  _setRef (ref) {
    // If you later need to use any public methods
    // You can: this._arrowKeyStepperRef.somePublicMethod()
    this._arrowKeyStepperRef = ref
  }
}

Were you thinking to expose such a method in as an additional property in the children(...) function?

We could potentially do that as well.

[alexandro81]

I see! ^_^
Well yes, I'm perfectly fine with that approach... it's simple enough and it works well.
Sorry I didn't think about the ref, but for some reason in my mind a ref was always referring to the underlying HTML element and not to the instance of the component (and of course that is wrong!) :)
I can go ahead and add that method with a PR if you're okay with that.

Thanks for the help!

[bvaughn]

No worries. I think that's a common thought-pattern about refs. 😄

Sure! I'd love to get a PR from you. Thanks for offering!

[alexandro81]

Here!
#592

Please let me know if you're gonna complete the PR or if there's something else I need to do first!

Thx!

[bvaughn]

Resolved by #592