Use callback state update for updates relying on previous (state, props)

[youknowriad]

Just trying to close #1117 #1118 #1119 #1120 #1121 #1122 #1123 #1124

When state updates rely on previous state or props, we should use callbacks. That's the latest recommendations (setState can be asynchronous). More details in the discussion here #1124

[aduth]

Still scratching my head as to where the line should be drawn between this being a theoretical issue vs. an actual issue.

[aduth]

This is an unfortunate a consequence from this sort of change. In general it'd be nice if we could at least create the function signature to include only those relevant to generating its result, not just accepting entire state and props of the instance. In this case it needs a fair few values (suggestions, incompleteTokenValue, value, max), so not sure if it's much of an improvement.

[youknowriad]

Anyway, we should make a decision, close all those issues or merge this PR :).
I'm hesitant too.

[BE-Webdesign]

Personally I like it, and it only adds two lines of code, which could even be eliminated. I am not overly partial either way, so if we close out the issues, and not merge, not a big deal. I believe this is a best practice now as well, so I lean more towards merging.

For #1124, it actually has made certain aspects of applying formats in the editable less buggy, when I am goofing around trying to break it. The selection stuff gets broken pretty easily still and I haven't had time to look into yet.

tl;dr I approve, but don't take that as a strong reason to merge or anything, I leave the final decision to aduth and yourself, or matias, someone other than me, as I am biased towards thinking this is better.

[youknowriad]

I've been thinking more about this. And I'm starting to lean towards merging this.

Not explored in this PR, but this pattern has a nice side-effect allowing the state changes to be declared (and tested) separately from the component itself. https://medium.freecodecamp.com/functional-setstate-is-the-future-of-react-374f30401b6b#2c7e

Btw, since we're already using Fiber, I think we should comply with its best practices even if this pattern is only needed if we use the async mode of Fiber (which is not enabled yet).

[aduth]

Okay, I'm not so much opposed to it, but but thinking we should come up with patterns for awkwardness-es like:

https://github.com/WordPress/gutenberg/blob/0c2bb45/components/form-token-field/index.js#L344-L346

In this case, either expecting state and props to be passed consistently, or updating the function signature to reflect the arguments relevant to suggestions being generated.

Aside: getMatchingSuggestions( state = this.state, props = this.props ) { works as an alternative for what exists currently.

[youknowriad]

Aside: getMatchingSuggestions( state = this.state, props = this.props ) { works as an alternative for what exists currently.

Yeah, wasn't sure about this. When the default value is resolved.

[aduth]

Yeah, wasn't sure about this. When the default value is resolved.

Not sure how the spec dictates, but at least Babel transforms it close to what exists in your implementation:

http://babeljs.io/repl/#?babili=false&evaluate=false&lineWrap=false&presets=es2015&targets=&browsers=ie%2011&builtIns=true&debug=false&code=function%20getMatchingSuggestions(%20state%20%3D%20this.state%2C%20props%20%3D%20this.props%20)%20%7B%7D&plugins=transform-runtime

[youknowriad]

thinking we should come up with patterns

I suspect that this kind of usage won't be so common, but regardless, I don't have any preference here. Happy to use the approach we settle on.

[BE-Webdesign]

Side note, #1124 doesn't actually make things better, it was my imagination lol. Not sure what happens but the apply formats is really inconsistent sometimes.

Update: I actually have no idea, because it does seem to work better when the function is used.

[aduth]

How can we enforce this?

[BE-Webdesign]

How can we enforce this?

Good question. Custom lint? I don't know. I will try and see if I can write a lint for it, never done it before though. I tried finding something that already does this and found nothing.

[youknowriad]

Do you agree with merging here as we work on the lint rule separately?

[aduth]

Sure 😄