algolia / algolia/instantsearch

Architecture refactor scout: github-32005295866

Open
#7,167 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
TypeScript
Stars
4.1k
Forks
553
Avg merge
1d 11h
Merged PRs (30d)
51

Description

cc @algolia/frontend-experiences-web

# Architecture Refactor Scout

Run: `github-32005295866`

## Summary

I scouted the whole monorepo with the module-depth rubric, concentrating on the layers where behavior actually lives: the shared connectors in `packages/instantsearch.js/src/connectors`, the runtime in `src/lib`, the highlight/snippet helpers, and how React and Vue wrap the same connectors. No `CONTEXT.md` or `docs/adr/` exists. The recurring friction is **shallow shared modules that force each caller to re-assemble the same orchestration**: the facet connectors (`refinementList`, `menu`, `hierarchicalMenu`) each hand-roll an identical show-more state machine; the "highlighted parts" pipeline is re-derived verbatim in all three flavors because no deep module owns "attribute → parts"; three refinement connectors each re-wrap `sendEvent` with the same "only emit on refine" guard; and a couple of purely-presentational widgets (e.g. `Stats`) duplicate a11y/announcement behavior between the legacy Preact components and the React `ui/` layer. Each finding below is a small, high-locality PR that pulls scattered orchestration behind one deeper interface.

## Candidate Shortlist

candidate-1: Deepen a show-more controller shared by the facet connectors

- Recommendation strength: `Strong`
- Files:
- `packages/instantsearch.js/src/connectors/refinement-list/connectRefinementList.ts` (~lines 240–259, 401–456)
- `packages/instantsearch.js/src/connectors/menu/connectMenu.ts` (~lines 185–201, 292–328)
- `packages/instantsearch.js/src/connectors/hierarchical-menu/connectHierarchicalMenu.ts` (~lines 218–238, 307, 398–413)
- Problem: All three facet connectors reimplement the *identical* show-more state machine: a mutable `isShowingMore` flag, a `toggleShowMore` placeholder, a `cachedToggleShowMore()` indirection (which exists purely to hand `renderFn` a **stable function identity** across renders — a subtle invariant every connector must remember), a `createToggleShowMore(renderOptions, widget)` that flips the flag and calls `widget.render()`, and a `getLimit()` that switches between `limit` and `showMoreLimit`. The interface each connector exposes (`isShowingMore`, `toggleShowMore`, `canToggleShowMore`) is small, but the implementation is copied byte-for-byte, so the invariants (stable identity, when to recreate the closure, how `canToggleShowMore` is derived from `getLimit()`) are re-encoded three times and can drift.
- Proposed change: Introduce one deep module that owns the show-more state — it holds `isShowingMore`, exposes a stable `toggleShowMore`, a `getLimit()`, and the derivation of `canToggleShowMore` — so each connector constructs it once and reads from it instead of re-declaring the machinery. The exact shape (factory vs. small class, how `render` is injected) is left for the implementation stage.
- Benefits: Locality — the show-more invariant (especially the stable-identity trick) lives in one file with one test instead of being duplicated across three connectors. Leverage — a connector opts into show-more behavior through a tiny interface. Testability — the state machine becomes directly unit-testable rather than only reachable through a full connector render cycle.
- Risks: Must preserve the stable-identity contract exactly (widgets and hooks rely on `toggleShowMore` keeping the same reference between renders); `hierarchicalMenu` slices nested `data`, so the limit application differs slightly and must stay parameterizable. Purely internal — no public interface change.
- Verification: Run the existing `connectRefinementList`, `connectMenu`, `connectHierarchicalMenu` unit suites plus `yarn jest common-widgets -t "RefinementList"` / `"Menu"` / `"HierarchicalMenu"`; assert `toggleShowMore` identity is stable across renders and that toggling flips `getLimit()`.

Before:

```mermaid
flowchart LR
RL[connectRefinementList] --> SM1[show-more state copy]
Menu[connectMenu] --> SM2[show-more state copy]
HM[connectHierarchicalMenu] --> SM3[show-more state copy]
```

After:

```mermaid
flowchart LR
RL[connectRefinementList] --> Ctrl[show-more controller]
Menu[connectMenu] --> Ctrl
HM[connectHierarchicalMenu] --> Ctrl
Ctrl --> Detail[isShowingMore / getLimit / stable toggle]
```

candidate-2: Own the highlight/snippet parts pipeline in one deep util

- Recommendation strength: `Strong`
- Files:
- `packages/instantsearch.js/src/helpers/components/Highlight.tsx`, `Snippet.tsx`, `ReverseHighlight.tsx`, `ReverseSnippet.tsx` (each ~lines 36–52)
- `packages/react-instantsearch/src/widgets/Highlight.tsx`, `Snippet.tsx`, `ReverseHighlight.tsx` (~lines 30–36)
- `packages/vue-instantsearch/src/util/parseAlgoliaHit.js`, `packages/vue-instantsearch/src/components/Highlighter.js`
- Underlying utils: `packages/instantsearch.js/src/lib/utils/{getHighlightedParts,getHighlightFromSiblings,reverseHighlightedParts,escape-highlight}.ts`
- Problem: There is no deep module for "given a hit + attribute, produce highlighted parts." Instead every flavor re-derives the same ordered pipeline by hand: `getPropertyByPath(hit._highlightResult | hit._snippetResult, attribute)` → normalize to array (`toArray` in JS, `Array.isArray` in React) → `getHighlightedParts(unescape(value))`, with the reverse variants additionally mapping through `reverseHighlightedParts`/`getHighlightFromSiblings`. The caller must know which result path to read, that `unescape` runs *before* splitting, how to normalize to an array, and (for reverse) the sibling-merge rule. That knowledge is duplicated across `instantsearch.js`, `react-instantsearch`, and `vue-instantsearch` — a leak across three seams that is drift-prone (React already normalizes arrays differently and drops the `warning`).
- Proposed change: Add one deep util in `instantsearch.js` that takes the hit, attribute, result kind (highlight vs. snippet), and reverse flag, and returns the finished parts — absorbing path selection, array normalization, unescape ordering, and the reverse-siblings logic. Each flavor helper then calls it and only renders. Exact signature deferred to implementation.
- Benefits: Leverage — four small tag/escape utils collapse behind one call the flavors share, so the normalization rules live once. Locality — a highlighting bug is fixed in one place for all flavors. Testability — the pipeline (including reverse sibling-merge edge cases) becomes the natural test surface instead of being re-tested per flavor.
- Risks: Cross-flavor change touching three published packages; must keep byte-identical output (highlight rendering is user-visible and snapshot-tested). React currently omits the `warning` and Vue routes through `parseAlgoliaHit` — behavior parity must be confirmed, not assumed. Best done additively (introduce the util, then migrate callers).
- Verification: `yarn jest common-widgets -t "Highlight"` / `"Snippet"`, plus the per-flavor `Highlight`/`Snippet` component snapshots in each package; diff rendered parts for mixed-entity and reverse cases before/after.

Before:

```mermaid
flowchart LR
JS[JS helpers] --> P1[re-derived parts pipeline]
React[React widgets] --> P2[re-derived parts pipeline]
Vue[Vue components] --> P3[re-derived parts pipeline]
```

After:

```mermaid
flowchart LR
JS[JS helpers] --> Deep[highlight-parts util]
React[React widgets] --> Deep
Vue[Vue components] --> Deep
Deep --> Detail[path / unescape / split / reverse-siblings]
```

candidate-3: Factor the refine-only sendEvent guard into one factory

- Recommendation strength: `Worth exploring`
- Files:
- `packages/instantsearch.js/src/connectors/rating-menu/connectRatingMenu.ts` (~lines 45–72)
- `packages/instantsearch.js/src/connectors/numeric-menu/connectNumericMenu.ts` (~lines 140–147)
- `packages/instantsearch.js/src/connectors/toggle-refinement/connectToggleRefinement.ts` (~lines 43–83)
- Shared util: `packages/instantsearch.js/src/lib/utils/createSendEventForFacet.ts`
- Problem: `createSendEventForFacet` covers generic facets, but `ratingMenu`, `numericMenu`, and `toggleRefinement` each hand-roll their own `createSendEvent` that wraps the base `sendEvent` with the same guard — "only emit a `click`/filter event when the value is *becoming* refined, not when it is being cleared" (`if (!isRefined) sendEvent(...)`). The only thing that varies is the `isRefined` predicate; the wrapping, event-name defaulting, and payload shape are copied. Callers can't tell that this refine-only semantics exists — it's baked separately into each connector, and a new filtering connector must copy it again.
- Proposed change: Extend the shared send-event helper (or add a sibling factory) so a connector supplies just the `isRefined` predicate and gets the guarded, correctly-defaulted `sendEvent` back, replacing the three near-identical local `createSendEvent` functions. Interface details deferred.
- Benefits: Locality — the "only fire on refine" rule and its default event names live in one tested place. Leverage — adding a new refinement connector means passing a predicate, not re-deriving event glue. Testability — the guard is unit-tested once at the util instead of implicitly through three connectors.
- Risks: The three connectors have subtly different argument tuples (e.g. `toggleRefinement` reads `isRefined` out of the args array; `ratingMenu` compares against `getRefinedStar()`), so the factory must accommodate those without changing emitted payloads; insights output is asserted in tests, so payload parity is critical.
- Verification: The `connectRatingMenu` / `connectNumericMenu` / `connectToggleRefinement` unit suites and any insights middleware tests that assert emitted event shape; confirm no event fires on un-refine and payloads are unchanged.

Before:

```mermaid
flowchart LR
Rating[connectRatingMenu] --> G1[refine-only guard copy]
Numeric[connectNumericMenu] --> G2[refine-only guard copy]
Toggle[connectToggleRefinement] --> G3[refine-only guard copy]
```

After:

```mermaid
flowchart LR
Rating[connectRatingMenu] --> F[sendEvent factory]
Numeric[connectNumericMenu] --> F
Toggle[connectToggleRefinement] --> F
F --> Detail[guard + defaults, predicate injected]
```

candidate-4: Move the Stats announcement behavior into the shared UI layer

- Recommendation strength: `Worth exploring`
- Files:
- `packages/instantsearch.js/src/components/Stats/Stats.tsx` (143 lines; `ANNOUNCEMENT_DELAY`, `visuallyHiddenStyle`, `aria-live` announcement effect ~lines 39–135)
- `packages/react-instantsearch/src/ui/Stats.tsx` (109 lines; same constants/effect ~lines 38–103)
- Target: `packages/instantsearch-ui-components/src/components/`
- Problem: `Stats` is a purely presentational widget, yet its accessibility behavior is duplicated between the legacy Preact component and the React `ui/` copy: the same `ANNOUNCEMENT_DELAY = 1400`, the same `visuallyHiddenStyle`, and the same debounced `aria-live="polite"` announcement effect. The repo has already standardized on `instantsearch-ui-components` owning shared markup (per `CLAUDE.md`), so this is a widget that has *not yet* been migrated — the announcement contract (delay, hidden style, live region) lives in two places and can silently diverge when the a11y spec changes.
- Proposed change: Extract a shared `Stats` factory (`createComponent({ createElement, Fragment })`) into `instantsearch-ui-components`, owning the class names, the visually-hidden announcement region, and the delay, then have both the JS and React layers consume it — the same migration path already used for newer widgets. Translations stay injectable.
- Benefits: Locality — the announcement/a11y behavior is defined once and ripples to every flavor (and `instantsearch.css`) at once. Leverage — flavors consume a factory instead of re-declaring markup + timing. Testability — the announcement timing/live-region is tested once in the shared package.
- Risks: Timing-dependent effect (debounced announcement) must behave identically under Preact and React; snapshot/markup and class names are user-facing and CSS-coupled, so structure must match exactly. This follows an existing decision rather than introducing a new pattern, but it is a cross-package move.
- Verification: `yarn jest common-widgets -t "Stats"`, the per-flavor `Stats` snapshots, and a fake-timer test asserting the announcement fires after `ANNOUNCEMENT_DELAY` with the correct `aria-live` region.

Before:

```mermaid
flowchart LR
JS[JS Stats component] --> A1[announcement + hidden style copy]
React[React ui/Stats] --> A2[announcement + hidden style copy]
```

After:

```mermaid
flowchart LR
JS[JS Stats component] --> Shared[shared Stats factory]
React[React ui/Stats] --> Shared
Shared --> Detail[a11y region + delay + classes]
```

candidate-5: Consolidate router + stateMapping wiring behind one seam

- Recommendation strength: `Speculative`
- Files:
- `packages/instantsearch.js/src/middlewares/createRouterMiddleware.ts` (~lines 47–126)
- `packages/instantsearch.js/src/lib/routers/history.ts`
- `packages/instantsearch.js/src/lib/stateMappings/{simple,singleIndex}.ts`
- Caller: `packages/instantsearch.js/src/lib/InstantSearch.ts` (~lines 403–406)
- Problem: `InstantSearch` already collapses `routing: true | object` into `createRouterMiddleware` in a few lines, so the *default* path is fine. The friction appears for a caller who wants a **custom** setup: they must instantiate `historyRouter` (which exposes ~16 options, only three commonly used), separately choose or implement a `stateMapping` (`simple` vs. `singleIndex`, and know that `configure` is filtered inside the mapping), and pass both into the middleware — three modules the caller assembles by hand, with knowledge (`configure` filtering, single- vs. multi-index mapping) leaking across their seams.
- Proposed change: Provide a single deeper entry that accepts the routing intent (custom router and/or mapping, write delay) and returns a ready middleware with sensible defaults selected internally, so callers stop wiring the router + mapping + middleware trio themselves. Concrete interface deferred; would need a genuine variation to justify the seam.
- Benefits: Leverage — custom routing becomes one call instead of three coordinated constructions. Locality — the "which mapping, filter `configure`, default write delay" knowledge concentrates behind the seam.
- Risks: Marked speculative because the default path is already deep and the public `historyRouter`/`stateMapping` exports are documented extension points — collapsing them risks reducing legitimate flexibility or introducing a wrapper that is itself pass-through. Any change must not alter emitted URLs. Validate that something actually varies across the new seam before committing.
- Verification: The routing middleware and `history` router unit suites plus the routing e2e specs (`tests/e2e`); assert identical URLs for default and custom-router configurations before/after.

Before:

```mermaid
flowchart LR
Caller[Caller] --> Router[historyRouter]
Caller --> Mapping[stateMapping]
Caller --> MW[createRouterMiddleware]
```

After:

```mermaid
flowchart LR
Caller[Caller] --> Seam[routing wiring seam]
Seam --> Router[historyRouter]
Seam --> Mapping[stateMapping]
Seam --> MW[createRouterMiddleware]
```

## Top Recommendation

Implement **candidate-1 (show-more controller)** first. It is the deepest, most local win: the show-more state machine — including the easy-to-miss stable-function-identity invariant — is duplicated verbatim across three sibling files in a single package, so pulling it behind one small interface has real depth (a lot of behavior and one tricky invariant behind `getLimit()`/`toggleShowMore`) with almost no blast radius. It changes no public interface, so migration and compatibility risk is minimal, and it is directly verifiable through the existing connector and `common-widgets` suites. The PR is small and self-contained, making it the safest way to establish the pattern before tackling the higher-leverage but cross-flavor candidate-2.

## Next Step

To trigger implementation of a selected candidate, run:

`/implement candidate-1`

Replace `candidate-1` with the id of the candidate you want to implement (e.g. `/implement candidate-2`).

## Non-Candidates

- **Auto-generating the ~31 React connector hooks** (`react-instantsearch-core/src/connectors/`): each hook is a thin one-line `useConnector(connectX, props)` pass-through, but they are the package's public, individually-typed, tree-shakeable exports. Replacing them with build-time generation is a broad rewrite that trades a real interface for machinery — rejected as too broad and low-depth.
- **A universal `UiStateCodec` / `SearchStateBuilder` across all connectors**: the per-connector `getWidgetUiState`/`getWidgetSearchParameters` encodings (range `"min:max"`, pagination offset, refinement cleanup) differ enough that a single codec abstraction risks being a speculative abstraction over genuinely different logic. Rejected per the "don't introduce a seam unless something actually varies cleanly across it" constraint.
- **Splitting `InstantSearch.ts` middleware composition + render scheduling into new managers**: tempting given the 924-line file, but the scheduling methods share private state and the middleware precedence rules are subtle; carving out `MiddlewareComposer`/`RenderScheduler` is a large, high-risk change to the core lifecycle, not a small module-deepening PR.
- **Migrating every legacy `instantsearch.js/src/components` widget (CurrentRefinements, Breadcrumb, RangeInput, …) to the shared UI layer**: this is a real, already-decided ongoing migration, but doing it wholesale is broad churn. Candidate-4 carves off the single smallest, purest instance (`Stats`) as one reviewable PR instead.

Contributor guide

Open the contributing guide

Research direction

Start by selecting one candidate and reading its listed connector or component files, such as the facet connectors or the Highlight/Snippet helpers. Run the candidate's existing unit and snapshot suites before changing anything. Done means one narrowly scoped shared abstraction replaces the duplication while preserving behavior and the listed tests pass.

Written by the indexing model from the issue text.

Assessment

Tech stack
react, typescript
Domain
developer-experience, frontend, tooling
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Needs clarification
Newbie friendliness
30/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.