Keep html-react-parser out of the core extension reader - #2061
Merged
Merged
Conversation
getTextDisplayFlyover lived in src/utils/extensions.ts, the module every
consumer goes through to read item extensions, and it called htmlParse to
return a React element. That single call made html-react-parser, and with it a
DOM assumption, a dependency of the whole extension-reading path. Headless
consumers of the engine (React Native, server-side, tooling) pull the UI layer
in just to ask whether an item has a flyover, and have to neutralise the module
to work around it.
Split the lookup at the data boundary:
- src/utils/extensions.ts gains getTextDisplayFlyoverSource, which walks the
flyover childItems exactly as before and returns plain data
({ xHtmlString, text }) instead of a React element. utils/extensions no
longer imports html-react-parser or the JSX type.
- src/hooks/useParseXhtml.tsx gains getTextDisplayFlyover, unchanged in name,
signature and behaviour, built on the new util. XHTML parsing now happens in
the UI layer, alongside the other parsing this module already owns.
- useRenderingExtensions imports it from its new home.
No file under src/utils, src/stores or src/api imports html-react-parser now.
The package still depends on html-react-parser, because the UI layer legitimately
uses it; the point is that it is no longer reachable from the headless graph.
RenderingExtensions.displayFlyover, FlyoverItem, ItemLabel, GroupHeading and
SdcUiOverrideComponentProps.displayText are untouched, so the rendered output is
identical. getTextDisplayFlyover was never re-exported from the package barrel,
so this only moves an internal module path.
The existing flyover tests move with the function: the parsing assertions now
run against hooks/useParseXhtml, and extension.test.tsx asserts the plain-data
shape.
getTextDisplayFlyoverSource keeps scanning later childItems when a flyover childItem carries neither a rendering-xhtml extension nor text. That branch was untested before the split, so pin it now that the lookup is plain data and cheap to assert.
Relocate getTextDisplayFlyover from utils/extensions to hooks/useParseXhtml unchanged, instead of splitting it into a data half and a parsing wrapper. The function has a single consumer (useRenderingExtensions), and the traversal only needs isSpecificItemControl and getXHtmlString, which stay exported from utils/extensions. This keeps html-react-parser out of the core extension reader with a smaller diff and drops the TextDisplayFlyoverSource interface introduced by the earlier split. Move the five pre-existing flyover tests verbatim to useParseXhtml.test.tsx, keep the fall-through test, and drop the wrapper tests that duplicated traversal coverage.
Collaborator
Verified in a combined test branchTo avoid manually testing three PRs separately, I built a local branch off This PR merged into Results on the combined branch:
Manual testing against that branch also looked good. One thing worth making explicitThe PR description presents the relationship to #2063 as a stacked base branch. It is stronger than that — #2061 is a hard functional dependency of #2063, and has to land first. #2063's So this is not just base-branch bookkeeping — merging #2063 without this one would land a red test. Worth noting for whoever sequences the merges. |
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.
Any code that reads a Questionnaire's extensions currently also pulls in an HTML parser, even when it never renders anything. This moves
getTextDisplayFlyover, the only function inutils/extensions.tsthat parses XHTML, intohooks/useParseXhtml.tsx, which already owns XHTML parsing.utils/extensions.tsbecomes a pure deletion: the function and thehtml-react-parserimport are removed, nothing else changes. It also removes one of the two things blocking the package from being marked tree-shakeable later.Fixes #2058
The function moved wholesale rather than being split into a data half and a parsing wrapper: it has a single consumer (
hooks/useRenderingExtensions.ts), and the traversal only needsisSpecificItemControlandgetXHtmlString, which remain exported fromutils/extensions.ts. A wholesale move keeps the parser out of the core extension reader with the smallest reviewable diff.For review: the function body in
useParseXhtml.tsxis identical to what was deleted fromextensions.ts(only the doc comment changed), and the five pre-existing flyover tests moved verbatim fromextension.test.tsxtouseParseXhtml.test.tsx, plus one new test pinning the fall-through to a later flyover childItem.One consumer-visible note:
getTextDisplayFlyoverwas never exported from the package barrel, but deep imports fromlib/utils/extensionswould break; a compatibility re-export would reintroduce the removed dependency.Testing: full renderer jest suite green (92 suites), eslint and prettier clean on touched files.
src/utils/choice.tsstill has one other hooks-layer import; left for a follow-up. No dependency or lockfile changes, so no conflicts with the sibling PRs.