Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add react-compiler export condition #177

Closed
wants to merge 1 commit into from
Closed

Conversation

stipsan
Copy link
Member

@stipsan stipsan commented Jun 19, 2024

The React Compiler were struggling to understand the flow of <PortableText>:

export function PortableText<B extends TypedObject = PortableTextBlock>({
value: input,
components: componentOverrides,
listNestingMode,
onMissingComponent: missingComponentHandler = printWarning,
}: PortableTextProps<B>): JSX.Element {
const handleMissingComponent = missingComponentHandler || noop
const blocks = Array.isArray(input) ? input : [input]
const nested = nestLists(blocks, listNestingMode || LIST_NEST_MODE_HTML)
const components = useMemo(() => {
return componentOverrides
? mergeComponents(defaultComponents, componentOverrides)
: defaultComponents
}, [componentOverrides])
const renderNode = useMemo(
() => getNodeRenderer(components, handleMissingComponent),
[components, handleMissingComponent],
)
const rendered = nested.map((node, index) =>
renderNode({node: node, index, isInline: false, renderNode}),
)
return <>{rendered}</>
}

And it got different results for its useMemo deps arrays that were used to memo renderNode, and thus opted out from optimizing it.

Here's a diff of what happens if you add react-compiler without refactoring anything, and with the changes in this PR: https://npmdiff.dev/%40portabletext%2Freact/3.0.18-canary.0/3.1.0-canary.2/package/dist/index.compiled.js/

What I like about the implementation on main is that if the components and onMissingComponent props were stable, then a changing value prop didn't result in getNodeRenderer and mergeComponents being called more than once. I wanted to preserve this behaviour, while also changing the code so that the compiler understands it and can auto memoize it further.

By splitting the work into two separate components we preserve the previous memoization, while adding a few new benefits:

  • Since <PortableText> is now wrapped in memo it can opt-out of looping a possibly very deep recursive tree of renderNode if parent components rerender but the props given to <PortableText> haven't changed.
  • It's easier to track why portable text components rerender, as react profiling will tell you that "PortableTextRenderer rerendered because prop 'renderNode' changed" instead of "PortableText rerendered because hook 2 changed"

@stipsan stipsan requested a review from rexxars June 19, 2024 13:53
@stipsan stipsan closed this Jun 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant