[sitecore-jss-react][sitecore-jss-nextjs] Prevent hydration error with dynamic components #1807
Conversation
…h dynamic components
packages/sitecore-jss-react/src/components/PlaceholderCommon.tsx
Outdated
Show resolved
Hide resolved
| const type = rendered.props.type === 'text/sitecore' ? rendered.props.type : ''; | ||
| if (!isEmpty) { | ||
| rendered = ( | ||
| <ErrorBoundary | ||
| key={rendered.type + '-' + index} | ||
| errorComponent={this.props.errorComponent} | ||
| componentLoadingMessage={this.props.componentLoadingMessage} | ||
| type={type} | ||
| isDynamic={(component as JssComponentType).isDynamic} | ||
| {...rendered.props} | ||
| > | ||
| {rendered} | ||
| </ErrorBoundary> | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
with this refactor does this mean PlaceholderMetadata component would not be included in the ErrorBoundary?
There was a problem hiding this comment.
Yes. The main goal of ErrorBoundary is to alert to problems with customer's and app's components. With this change the focus is moved on these components specifically and our built-in stuff will not affect error boundaries.
There was a problem hiding this comment.
but what if an error occurs in the PlaceholderMetadata component?
There was a problem hiding this comment.
Then we have a larger problem on our side and each page of the site stops working in editing mode.
Which would be roughly the same behavior if PlaceholderMetadata errors are handled by ErrorBoundary - the content will not be shown.
| it('should not render Suspense and default loading message when wrapping a dynamic component', async () => { | ||
| // mount fails with lazy component and no suspense | ||
| const rendered = mount( | ||
| <Suspense> |
There was a problem hiding this comment.
Does this mean we have to wrap all the dynamic components with Suspense?
There was a problem hiding this comment.
for tests, yes
mount here refers to enzyme mount() specifically
…63-hydration-error-dynamics # Conflicts: # CHANGELOG.md
| component as React.ComponentType, | ||
| this.props.modifyComponentProps ? this.props.modifyComponentProps(finalProps) : finalProps | ||
| ); | ||
|
|
||
| const type = rendered.props.type === 'text/sitecore' ? rendered.props.type : ''; | ||
| if (!isEmpty) { |
There was a problem hiding this comment.
@art-alexeyenko Previously we had some comments here explaining problems, it should be reverted back
| component as React.ComponentType, | ||
| this.props.modifyComponentProps ? this.props.modifyComponentProps(finalProps) : finalProps | ||
| ); | ||
|
|
||
| const type = rendered.props.type === 'text/sitecore' ? rendered.props.type : ''; |
There was a problem hiding this comment.
@art-alexeyenko Should be added under if statement
| @@ -5,3 +5,11 @@ import { ComponentType } from 'react'; | |||
| * @param {string?} exportName component to be imported in case you export multiple components from the same file | |||
| */ | |||
| export type ComponentFactory = (componentName: string, exportName?: string) => ComponentType | null; | |||
There was a problem hiding this comment.
@art-alexeyenko Here ComponentFactory returns ComponentType while you introduced a new JssComponentType. Here is a mismatch from my understanding
| component as React.ComponentType, | ||
| this.props.modifyComponentProps ? this.props.modifyComponentProps(finalProps) : finalProps | ||
| ); | ||
|
|
||
| const type = rendered.props.type === 'text/sitecore' ? rendered.props.type : ''; | ||
| if (!isEmpty) { |
There was a problem hiding this comment.
@art-alexeyenko Add a comment here explaining that components are not wrapped in Chromes Edit Mode
Description / Motivation
Fixes hydration issues when ErrorBoundary's Suspense wraps nextjs's dynamic() wrapper.
dynamic() components will have a
renderprop. If it's present - the component will not be wrapped in ErrorBoundary'sSuspensepreventing intermittent hydration problems.Testing Details
Types of changes