Agree on limiting usage of this and guarding against setTimeout. 👍

At first I thought the editor.removed check was still needed in editable, but by then the timeout should be cleared anyway, so this seems useful there too.

Do you think it should be called setSafeTimeout or just setTimeout, if it comes from props and if we may want to expand it?

Do you think it should be called setSafeTimeout or just setTimeout

Been wondering this too. Pro of setSafeTimeout: the name makes it obvious there's something inherently different about this, and suggests a preferred method. Pro of setTimeout: makes it clear that it can and should be used the same way as window.setTimeout. Second pro: brevity. ;)

Let's try setTimeout unless someone feels strongly otherwise.

if it comes from props and if we may want to expand it?

What do you mean? Expand as in wrap the function, or expand as in accommodate its friends setInterval, etc.?

Yeah in this case I'm leaning towards setTimeout. :) And yes, I meant expand with debounce etc.

One issue I might see is that it introduces variable shadowing with the global setTimeout if one were to destructure, which also has the side-effect of potentially misleading a casual reader into what the setTimeout actually is:

const { setTimeout } = this.props;

// Later...

setTimeout( ... );

The latter point could be perceived as a benefit, since the behavior should be identical anyways to the global, but there is magic involved all the same.

One issue I might see is that it introduces variable shadowing with the global setTimeout

That's true, though for me it highlights an equivalent but more common problem, which is identifier collision between an action creator and its bound counterpart:

import { doThing } from 'actions'
connect(
  null,
  { doThing }
)( ( { doThing } ) => ( // oops, eslint go sad
  <button onClick={ doThing } />
) )

Gutenberg has, for the most part, avoided this by using its weird action naming convention:

connect( null, { onDoThing: doThing } )( … )

which is a whole other topic and which I personally don't like (in this example, I'd either go for doThing or onClick as bound action creator names), but projects like Calypso are still subject to collision. In Calypso, a pattern I commonly see (and use myself) is to distinguish data props from action props:

render() {
  const { foos } = this.props;
  return <button onClick={ this.props.doThing }>{ foos }</button>;
}

In Gutenberg, where functional components are more common, I could see this emerge:

import { getFoos } from 'selectors'
import { doThing } from 'actions'
connect(
  ( state ) => ( { foos: getFoos( state ) } ),
  { doThing }
)( ( { foos, ...actions } ) => (
  <button onClick={ actions.doThing }>{ foos }</button>
) )

The point of this detour is that I'd welcome a convention that would have us refer to effectful functions with some sort of namespacing (actions.doThing, props.doThing, this.props.doThing…).

Gutenberg has, for the most part, avoided this by using its weird action naming convention:

While I'd be lying if I said the convention wasn't at all related to the shadowing issue, I don't entirely agree in it being "weird" if considering the implied distinction between presentational and container components. button need not know of doThing being the result of its click — where doThing is more likely something like replaceBlock — since this is the role of the connect mappings to map the presentational behavior of the click (or equivalently named presentational behavior) to the state effects.

I'd agree though that this is often forced and at best still a pain point in props naming.

In Gutenberg, where functional components are more common, I could see this emerge:

One practical concern I might have here can be seen in how this transpiles via Babel:

http://babeljs.io/repl/#?babili=false&browsers=&build=&builtIns=false&code_lz=GYVwdgxgLglg9mABAWQJ4GE4FsAOCCmYUAFIgN6LBxwDOANIgHTMCG08YNiAvogJTluQA&debug=false&forceAllTransforms=false&shippedProposals=false&circleciRepo=&evaluate=true&fileSize=false&lineWrap=false&presets=latest%2Creact%2Cstage-2&prettier=false&targets=&version=6.26.0&envVersion=

Note the iteration.

Obviously I'd not be fond of letting the specifics of the transpile dictate how code is best written, but it can be a factor, or at least worth noting.

One practical concern I might have here can be seen in how this transpiles via Babel:

Thanks for checking, it's useful for weighing the discussion.

considering the implied distinction between presentational and container components

I agree with this, and appreciate the decoupling in general. My criticism is targeted at this pattern:

connect( null, { onMultiSelect: multiSelect } )
// later, in the component:
expandSelection( … ) {
  …
  this.props.onMultiSelect( … );
}

onMultiSelect is simply the action's name disguised as an event handler name, meaning it is actually coupling event/trigger and action/effect. This is why I initially said I'd have preferred to pick either { multiSelect: multiSelect } or { onSelectionExpanded: multiSelect } (the latter would likely require some custom logic, e.g. { onSelectionExpanded: ( x, y ) => multiSelect( foo( x ), bar( y ) ) }). What I meant by "weird"—and meaning no offense—was this application of "handler case" to an action's name.

Pushed 4500940 ff410da, which keeps this.timeouts trim by removing IDs as soon as their function calls are due. @iseulde, is this what you had in mind?

The behavior is illustrated by the following screencast, where I intentionally added a couple of seconds to the setTimeout calls (which were using 0 as the delay) to make them visible:

• Opening and closing the ellipsis menu: the unmount clean-up reveals no pending timeouts
• Clicking "Copy" and waiting: the timeout is removed as soon as 'copied' is logged
• Clicking "Copy" several times and waiting: the queued timeouts are successively removed
• Clicking "Copy" several times and closing the ellipsis menu: a batch of timeouts are removed

Pushed 4500940, which keeps this.timeouts trim by removing IDs as soon as their function calls are due. @iseulde, is this what you had in mind?

Yes, this is what I did in #4603 as well. :)

I wonder if this PR should also replace (some) setTimeout uses in e.g. the richText component?

This is why I initially said I'd have preferred to pick either { multiSelect: multiSelect } or { onSelectionExpanded: multiSelect } (the latter would likely require some custom logic, e.g. { onSelectionExpanded: ( x, y ) => multiSelect( foo( x ), bar( y ) ) }). What I meant by "weird"—and meaning no offense—was this application of "handler case" to an action's name.

Oh, no offense taken at all. I'd agree with you, and your example of onSelectionExpanded: multiSelect is indeed what I was getting at. As you point out, and purely on the basis of naming these "correctly", it's not a simple task certainly vs. some predictable pattern.

I wonder if this PR should also replace (some) setTimeout uses in e.g. the richText component?

That sounds good, though I can't do it now (have to step out for the evening). I can take care of it tomorrow, or feel free to add those.

@iseulde, I've pushed e571c35. Does that match your expectations? If so, is this ready to go?

Yes. I wonder if editor.removed is still needed... Theoretically it is only removed on unmount, and then the timer is cleared.

Feedback addressed; final review?

Stumbled upon some logic in embeds, wondering if we should offer a similarly bound version of fetch: