Search component: simplify type annotations

[jsnajdr]

A little followup to #50004.

First, to create a searchInput ref with correct inferred type, the following syntax is more concise:

const searchInput = React.useRef< HTMLInputElement >( null );

The type param doesn't need to be HTMLInputElement | null. The * | null part is implied by the RefObject typedef:

interface RefObject<T> {
  readonly current: T | null;
}

This also removes the need to cast the ref prop with as RefObject< ... > when passing to a component.

Note that at least for the searchInput ref, which is always assigned, we could make use of the non-null assertion operator: searchInput.current!.focus(). But non-null assertions are disabled with ESLint. I believe they would be useful for many React situations like this.

Second, I moved the imperative handle methods to be inline in the useImperativeHandle call. Makes the code much simpler and removes a few useCallback usages.

Third, I removed several void and JSX.Element return type declarations. They are very easy to infer.

[matticbot]

Test live: https://calypso.live/?branch=simplify/search-types

[matticbot]

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

App Entrypoints (~43 bytes removed 📉 [gzipped])

name                 parsed_size           gzip_size
entry-gutenboarding        -93 B  (-0.0%)      -43 B  (-0.0%)

Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used.

Sections (~115 bytes removed 📉 [gzipped])

name      parsed_size           gzip_size
settings        -93 B  (-0.0%)      -33 B  (-0.0%)
plugins         -93 B  (-0.0%)      -41 B  (-0.0%)
account         -93 B  (-0.0%)      -41 B  (-0.0%)

Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to.

Async-loaded Components (~108 bytes removed 📉 [gzipped])

name                                parsed_size           gzip_size
async-load-quick-language-switcher        -93 B  (-0.1%)      -34 B  (-0.1%)
async-load-design-playground              -93 B  (-0.0%)      -33 B  (-0.0%)
async-load-design-blocks                  -93 B  (-0.0%)      -33 B  (-0.0%)
async-load-design                         -93 B  (-0.0%)      -33 B  (-0.0%)
async-load-automattic-search              -93 B  (-0.1%)      -41 B  (-0.1%)

React components that are loaded lazily, when a certain part of UI is displayed for the first time.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

[tyxla]

Seems like declaring focus here will cause unexpected results - we're expecting that it's declared because we're calling it here.

[jsnajdr]

Good catch! It's weird that ESLint is not complaining about undeclared identifier. I'll check if this particular call in openSearch needs the timeout or if we can call searchInput.current.focus() directly.

[tyxla]

I guess it doesn't complain because it expects that you meant window.focus() 😉

[jsnajdr]

I see. For JavaScript and ESLint, we disabled these window.* globals (there are hundreds of them) in #37953. We'll need some TypeScript solution for the same problem.

Related TS issue: microsoft/TypeScript#14306

[jsnajdr]

I did a bit of research about why we need the setTimeout wrapper on the focus call and figured out we can remove it.

It was originally added by @blowery in #7883 to solve a bug with <Search autoFocus /> inside a <Popover />. At that time, autofocus was implemented by calling .focus() inside componentDidMount. And at time of mount, the Popover wasn't done positioning yet -- it started measuring elements and figuring out its position also in componentDidMount. So we needed to delay the .focus() call a little bit.

Today, we merely pass the autoFocus prop to the inner native <input> element and the buggy .focus() calls no longer happen. So I removed the setTimeout in 917e843, simplifying the code nicely.

[sarayourfriend]

Thanks for this PR. It LGTM aside from the one issue that Marin pointed out. I suspect that's the cause of the failing unit test as well 🙂

[sarayourfriend]

LGTM! Thanks 🌵 🤠