Giant search: switch search.js and searchBox.js to typescript (no class-function conversion)#737
Merged
Conversation
hoyla
added
search
maintenance
hoyla
marked this pull request as ready for review
May 18, 2026 08:37
Contributor
Author
|
Phil: do the JS->typescript migration with the bot but not the conversion from a class based to functional component |
hoyla
marked this pull request as draft
May 18, 2026 09:48
The file previously exported only a PropTypes shape. Add a proper TypeScript interface alongside so TS consumers have a single canonical source for the suggested-field shape, rather than each component file re-declaring it inline. The PropTypes shape is retained for the existing JS consumers (InputSupper, Chip).
Add the two remaining fields that the search reducer manages but the
type previously elided: suggestedFields (initialised to [] and
populated by SUGGESTED_FIELDS_GET_RECEIVE) and searchFailed (toggled
by SEARCH_GET_REQUEST and SEARCH_FAILURE). The TODO comment is
removed since the omitted fields are now declared.
The shape of currentQuery remains { q: string } even though the
reducer actually stores the full search-query payload there;
tightening that is a separate concern.
The page and pageSize URL params have always been strings at runtime (URL query parameters are inherently strings, and every call site in the frontend passes string literals like "1" or option values like "25"). The types previously claimed number in three places, which forced consumers to either cast at the call site or absorb a silently-wrong type. Three coordinated fixes, all narrowing types to match the runtime: - updatePage / updatePageSize action creators now take string. - The corresponding action payload types in GiantActions drop their any annotations in favour of string. - UrlParamsState.pageSize moves from number to string, matching the already-string UrlParamsState.page. No behavioural change. The reducer was already storing whatever the action carried.
The backend search endpoint has always returned pageSize in its response, and the search reducer reads action.searchResults.pageSize when computing total pages. The TS type omitted the field, so any consumer reading currentResults.pageSize tripped a TS2339 in strict projects. Add the field to both the TypeScript type and the corresponding PropTypes shape.
Pure types-only migration: same class structure, same lifecycle, same methods. Adds a Props interface and an internal InputSupperHandle type for the ref. PropTypes are removed since the TS interface supersedes them. One trivial dead-code drop: the original focus()/select() guarded on this.searchInput !== document.activeElement, but searchInput holds a React component instance, not an HTMLElement, so the comparison was always true. The accompanying "value reset" hack set .value on the same React instance, which had no effect. Simplified to a plain null-check; observable behaviour is identical.
Pure types-only migration: same class structure, same lifecycle
methods (componentDidMount, UNSAFE_componentWillReceiveProps,
componentDidUpdate, componentWillUnmount), same _debounce class field,
same connect() + bindActionCreators wiring. Adds StateProps,
DispatchProps, and SearchState interfaces; replaces PropTypes with
those interfaces; uses ThunkDispatch in mapDispatchToProps so the
thunk action creators type-check.
Three small drops, each behaviour-preserving:
- The `addQuery={this.addQuery}` prop passed to <SearchBox>: this.addQuery
was never declared on the class (always undefined at runtime), and
SearchBox didn't declare or use the prop either. Dead on both sides.
- The renderControls <Select> components previously passed value as a
plain string. react-select v1's @types declare value as ValueType
(an option object), so we look up the current option from the
options array and pass that. At runtime react-select v1 resolves a
string the same way; the option lookup is the type-honest
equivalent.
- searchResultsPropType and suggestedFieldsPropType imports are
dropped along with the PropTypes block they fed.
The class-based structure is intentionally preserved; converting to
hooks-and-functional form is a separate concern and would happen in
a follow-up PR.
The debounced text updater dispatches updateSearchText(text) and then calls triggerSearch(this.props.urlParams). The dispatched action updates urlParams.q asynchronously, so the immediate triggerSearch call was using the stale q from before the user typed — searching with empty/old text. Pressing Enter a second time worked because by then the dispatched state had landed. Pass the freshly-typed text in the query object directly so the search fires with the user's actual input on the first Enter.
hoyla
force-pushed
the
ljh-search-js-to-ts
branch
from
May 18, 2026 10:48
1aeffe6 to
37cad58
Compare
hoyla
marked this pull request as ready for review
May 18, 2026 11:14
hoyla
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pure types-only migration of the search page (
Search.js/SearchBox.js) from JavaScript class components to TypeScript class components, plus a handful of small upstream type fixes that the migration surfaced.Redone after review feedback: removes the conversion from class based components to functions because it was too much in one PR - see comment from Phil below.
Precursor to the ljh-searchux-v2 PR stack (#653, #662, #663, #664, #665) — lifting the migration out of the v2 feature work means those feature PRs become smaller and purely additive. Stands on its own as a refactor: even if the v2 stack never lands, the search page now has proper TypeScript types and three latent type inconsistencies are fixed.
What's in the PR
Seven commits in logical order:
types/SuggestedFields.jsto TypeScript — exports aproper
SuggestedFieldinterface alongside the existingPropTypesshape.SearchStatetyping inGiantState— addssuggestedFields: SuggestedField[]andsearchFailed?: boolean,which the reducer manages but the type previously elided.
updatePage/updatePageSizesignatures, matching action payloadtypes, and
UrlParamsState.pageSizeall becomestring, matchingevery existing call site.
pageSizetoSearchResultstype — the runtime API hasalways returned it and the search reducer reads it.
SearchBox.jsto TypeScript — same class structure,same methods. Adds
Propsinterface and a smallInputSupperHandleinterface for the ref.
Search.jsto TypeScript — same class structure, samelifecycle methods (including
UNSAFE_componentWillReceiveProps),same
_debounceclass field, sameconnect()+bindActionCreators. AddsStateProps,DispatchProps, andSearchStateinterfaces. UsesThunkDispatchinmapDispatchToPropsso thunk action creators type-check.bug surfaced while smoke-testing the migration: the debounced
search was firing with the stale URL
qrather than thefreshly-typed text.
What this PR is not
connect()touseDispatch/useSelector.react-selectupgrade. v1 is still pinned; a type-honestoption-lookup pattern replaces the previously-passed-as-string
valueprop.Verification
npx tsc --noEmit: no new errors (only the 3 pre-existing@types/lodashbaseline).cd frontend && CI=1 BROWSER=none npx react-scripts build:compiles with the pre-existing
@elastic/chartssource-mapwarnings; no errors.
cd frontend && CI=1 npx react-scripts test --watchAll=false:23 suites / 204 tests passing.
Test plan
/searchcold — initial search fires with the URLq/search— title resets to "Giant"