refactor: Deepen highlight-parts transformation into one module#7091
Draft
github-actions[bot] wants to merge 1 commit into
Draft
Conversation
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 21 |
TIP This summary will be updated as you push new changes.
More templates
algoliasearch-helper
instantsearch-ui-components
instantsearch.css
instantsearch.js
react-instantsearch
react-instantsearch-core
react-instantsearch-nextjs
react-instantsearch-router-nextjs
vue-instantsearch
commit: |
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
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.
Summary
Architecture Refactor Implementation: candidate-1
Run:
github-28094741786Branch:refactor/candidate-1-deepen-highlight-parts-transformation-into-one-mSelected Approach
Chosen: one unified
highlight-parts.tsmodule, with the four original files kept as thin re-export shims.The "parse / serialize / reverse a highlighted string" concept was split across four
tiny exports (
getHighlightedParts,concatHighlightedParts,reverseHighlightedParts,getHighlightFromSiblings), two of which independently importedTAG_REPLACEMENTfrom afifth file. I moved the implementation of all four functions into a single deep module,
packages/instantsearch.js/src/lib/utils/highlight-parts.ts, so the inverse operations(
getHighlightedParts⇄concatHighlightedParts), the reverse operation, the siblingdefault rule, and the single
TAG_REPLACEMENTcoupling now live in one place. The fouroriginal files became one-line re-export shims so every public/barrel name and every deep
import path (
instantsearch.js/es/lib/utils/getHighlightedParts, etc.) keeps resolvingunchanged.
As part of the consolidation I fixed the latent sibling-default bug in
getHighlightFromSiblings:parts[i + 1]?.isHighlighted || true/parts[i - 1]?.isHighlighted || truecollapsed every presentfalseneighbor totrue, so the function effectively always returnedtruefor a non-alphanumeric part.Replacing
|| truewith?? truepreserves the intended "missing sibling defaults tohighlighted" behavior while honoring a present
falseneighbor.Why this approach: It is the lowest-risk option that still deepens the module. All
external consumers (react-instantsearch, vue-instantsearch, and the instantsearch.js
helpers/) import these symbols through thelib/utilsbarrel, which is untouched. Theshims also keep undocumented deep-import subpaths working, so there is zero migration cost
for any consumer. The implementation is now concentrated in one cohesive module — a
tag-format or sibling-rule change touches one file instead of five.
Rejected alternatives:
highlight-parts.ts. Slightlycleaner (no pass-through files), but it removes the per-file deep-import subpaths from
the published
es/build and forces churn in the existing test files. Marginal benefitfor added compat risk, so rejected in favor of shims (which the scout report explicitly
proposed: "keep the existing named exports as thin re-exports").
getHighlightFromSiblingsfully private (drop its public/barrel export). Thisbest matches the "hide the sibling logic" framing, but
getHighlightFromSiblingsiscurrently exported from the
lib/utilsbarrel, so removing it is a public-surfacechange with no functional payoff. Kept it exported; it is now simply colocated with the
reverse logic it serves.
TAG_REPLACEMENTinto the new module.escape-highlight.tslegitimately ownsthat constant (it also drives
replaceTagsAndEscape/escapeFacets), so moving it wouldwiden the diff and couple escaping to highlight-parts. Left it where it is; the new
module imports it once instead of from two separate files.
Summary
Consolidated the four highlight-parts utilities into a single deep module while preserving
every public export and import path via thin re-export shims, and fixed the
getHighlightFromSiblings|| truesibling-default bug (|| true→?? true) so anon-alphanumeric separator correctly adopts the shared state of present non-highlighted
siblings.
Changed Files
Source:
packages/instantsearch.js/src/lib/utils/highlight-parts.ts(new) — unified moduleholding
getHighlightedParts,concatHighlightedParts,getHighlightFromSiblings, andreverseHighlightedParts, with the singleTAG_REPLACEMENTimport and the corrected?? truesibling default. Functions carry brief JSDoc describing the inverse/reverserelationships.
packages/instantsearch.js/src/lib/utils/getHighlightedParts.ts— now re-exportsgetHighlightedPartsfrom./highlight-parts.packages/instantsearch.js/src/lib/utils/concatHighlightedParts.ts— now re-exportsconcatHighlightedPartsfrom./highlight-parts.packages/instantsearch.js/src/lib/utils/reverseHighlightedParts.ts— now re-exportsreverseHighlightedPartsfrom./highlight-parts.packages/instantsearch.js/src/lib/utils/getHighlightFromSiblings.ts— now re-exportsgetHighlightFromSiblingsfrom./highlight-parts.Tests:
packages/instantsearch.js/src/lib/utils/__tests__/highlight-parts-test.ts(new) —round-trip test asserting
concat(get(x)) === xacross single-match, multi-match,fully-highlighted, and no-highlight inputs, exercised at the unified module's surface.
packages/instantsearch.js/src/lib/utils/__tests__/getHighlightFromSiblings-test.ts—added cases pinning the corrected behavior: present
falsesiblings keepfalse(the bug fix), a missing sibling still defaults to highlighted, and disagreeing siblings
preserve the part's own state.
packages/instantsearch.js/src/lib/utils/__tests__/reverseHighlightedParts-test.ts—updated the multi-match expectation: the
-separator between two reversed-highlightedsiblings is now
true(contiguous) instead of the previous buggyfalse, with a commentexplaining the change.
Expected Validation
Ran in this workflow:
yarn install --frozen-lockfile— dependencies were absent; installed once. Succeeded.yarn lint:changed— reported "No lintable files matched" because it diffsmerge-base(HEAD, origin/master)..HEAD, and the change is intentionally leftuncommitted. Attempts to lint the explicit paths (
yarn lint:ox <files>/lint:staged) required interactive command/git addapproval that is unavailable inthis workflow, so oxlint did not run on the changed files here. The changes are
stylistically identical to the pre-existing code (moved verbatim plus JSDoc), so lint
risk is low; PR CI must run oxlint over these files.
yarn jest --findRelatedTests <5 changed source files> --ci --runInBand --passWithNoTests— 296 suites / 3785 tests passed (237 skipped). This includes all four util test
files, the new
highlight-parts-test.ts, and the helperreverseHighlight/reverseSnippetsnapshot tests, confirming the?? truefix changes no real-worldrendered output (the degenerate input it affects cannot be produced by
getHighlightedParts, which never emits consecutive non-highlighted parts).Not run (left to PR CI), with rationale:
yarn type-check— the candidate does not change any shared TypeScript contract,generated declaration, public export name, or cross-package API: the
lib/utilsbarrelstill exports the same four symbols with identical signatures, and
HighlightedPartsis unchanged. The round-trip test's
concat(get(x))is type-correct becausegetHighlightedPartsreturns{ value: string; isHighlighted: boolean }[], which isstructurally
HighlightedParts[]. Per the validation rules, full type-checking is leftto PR CI.
behavior-preserving refactor.
Review Notes
getHighlightFromSiblingsfix (|| true→?? true).In real usage this is reached only via the instantsearch.js
reverseHighlight/reverseSnippetstring helpers (the React/Vue/preact reverse components do a naiveper-part flip and never call this function). The only input shape whose output changes is
a non-alphanumeric part sitting between two present
falsesiblings — whichgetHighlightedPartsnever emits — so all existing helper snapshots are unchanged(verified green). The one assertion that changed is the synthetic
reverseHighlightedPartsunit test, which previously encoded the buggy output.subpath. If a reviewer prefers no pass-through files, the follow-up would be to delete
the shims and re-point the barrel at
highlight-parts.ts— deliberately deferred here tokeep migration risk at zero.
TAG_REPLACEMENTout ofescape-highlight.ts, and the@MAJORTODO about usingTAG_PLACEHOLDERinstead ofTAG_REPLACEMENTingetHighlightedParts— both are pre-existing and unrelated to thisdeepening.
Source