Add withSafeTimeout higher-order component

[mcsf]

Description

Sparked by recent in situ conversations and by #4603, here's an attempt to make asynchronous function calling safer using a lifecycle-bound setTimeout equivalent.

Depending on use cases, I could also see this accommodating more functions (setInterval, perhaps even debounce), whether via HoC arguments or separate HoCs.

How Has This Been Tested?

1. Apply this patch to tweak CopyContentButton's behavior.
2. Go to the editor, open the console.
3. Open the global ellipsis menu. Click Copy All Content.
4. Leave the menu open. Make sure that, after a second, the console logs copied.
5. Open the ellipsis menu again. Click Copy All Content and immediately close the menu.
6. Make sure that the console doesn't log copied after that.
7. In editor/hooks/copy-content/index.js, replace the setSafeTimeout( … ) call with the standard window.setTimeout. Repeat step 5 and make sure that the message gets logged even though the menu was previously closed.

Screenshots (jpeg or gifs if applicable):

Types of changes

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code has proper inline documentation.

[ellatrix]

Do you think we should remove finished timeouts? Or should we just let them pile up?

[mcsf]

I was picturing more short-lived instances, but good point: we should be defensive anyway.

[aduth]

Cool 👍 Agree with @iseulde's point.

Related but not necessary here: Might be worth exploring ESLint rules restricting non-HOC use of timers in components, or even also warning about safe usage (i.e. still expecting them to be used in some cases, but discouraging them as a crutch)

[aduth]

Component is unused.

[mcsf]

Hawk eye!

[mcsf]

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

[ellatrix]

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?

[mcsf]

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.?

[ellatrix]

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

[aduth]

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.

[mcsf]

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…).

[aduth]

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.

[mcsf]

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.

[ellatrix]

Should we return the timeoutID, in case someone wants to clear the timeout not just on unmount? Should we pass clearTimeout as well?

[mcsf]

Should we return the timeoutID

Absolutely, which I just noticed I didn't do.

Pass clearTimeout

This I'm not so sure. What would we provide? The same as window.clearTimeout?

[ellatrix]

Yes. It might be more convenient to get it from the same place. Without it I'd need to get one from props, and "import" the other from the window. Maybe it's also more future-proof.

[ellatrix]

Also, I guess to be super correct, it would also need to call this.clear( id ).

[mcsf]

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

[ellatrix]

Any reason not to shorten this to the following?

this.timeouts = this.timeouts.filter( t => t !== id );

[ellatrix]

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?

[aduth]

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.

[mcsf]

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.

[mcsf]

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

[ellatrix]

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

[aduth]

Remove console.log ?

[aduth]

Technically I think it'd be fine if the logic of clear was included here, than done separately, since clearTimeout on a finished timeout should be a noop (the reason I'm guessing you separated clear).

[mcsf]

Yep, the instinct was the separation, but we can merge.

[aduth]

Maybe simpler (and avoiding unlegible abbreviations 😬 ): https://lodash.com/docs/4.17.4#without

[mcsf]

Heh, yes. A consequence of quick prototyping.

[mcsf]

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

Yes, sounds about right. Wanted to check with you. :)

[mcsf]

Feedback addressed; final review?

[ellatrix]

Wonderful :)

[mcsf]

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

gutenberg/blocks/library/embed/index.js

Lines 86 to 89 in e539ea0

	componentWillUnmount() {
	// can't abort the fetch promise, so let it know we will unmount
	this.unmounting = true;
	}