-
Notifications
You must be signed in to change notification settings - Fork 391
feat(hooks): introduce <DynamicWidgets>
#3216
Conversation
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 69b6cd2:
|
✔️ Deploy Preview for react-instantsearch ready! 🔨 Explore the source changes: a608a74 🔍 Inspect the deploy log: https://app.netlify.com/sites/react-instantsearch/deploys/61a4eb71f77d53000801c91b 😎 Browse the preview: https://deploy-preview-3216--react-instantsearch.netlify.app |
invariant( | ||
React.Children.count(component.props.children) === 1, | ||
`<DynamicWidgets> only supports a single component in nested components. Make sure to not render multiple children in a parent component. | ||
|
||
Example of an unsupported scenario: | ||
|
||
\`\`\` | ||
<DynamicWidgets> | ||
<MyComponent> | ||
<RefinementList attribute="brand" /> | ||
<Menu attribute="categories" /> | ||
</MyComponent> | ||
</DynamicWidgets> | ||
\`\`\` | ||
` | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a new invariant that we don't yet have in the other flavors. It provides guidance on what to do in this scenario because React.Children.only
throws a generic error when there are multiple children.
// There are situations where InstantSearch.js may render widgets slightly | ||
// after they're removed by React, and thus try to update the React state | ||
// on unmounted components. React 16 and 17 consider them as memory leaks | ||
// and display a warning. | ||
// This happens in <DynamicWidgets> when `attributesToRender` contains a | ||
// value without an attribute previously mounted. React will unmount the | ||
// component controlled by that attribute, but InstantSearch.js will stay | ||
// unaware of this change until the render pass finishes, and therefore | ||
// notifies of a state change. | ||
// This ref lets us track this situation and ignore these state updates. | ||
if (shouldSetStateRef.current) { | ||
const { instantSearchInstance, widgetParams, ...renderState } = | ||
connectorState; | ||
|
||
// eslint-disable-next-line @typescript-eslint/no-use-before-define | ||
setState(renderState); | ||
}); | ||
// eslint-disable-next-line @typescript-eslint/no-use-before-define | ||
setState(renderState); | ||
} | ||
}, | ||
() => { | ||
// We'll ignore the next state update until we know for sure that | ||
// InstantSearch.js re-inits the component. | ||
shouldSetStateRef.current = false; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the important part that avoids memory leaks warnings in React 16 and 17.
✔️ Deploy Preview for react-instantsearch ready! 🔨 Explore the source changes: 69b6cd2 🔍 Inspect the deploy log: https://app.netlify.com/sites/react-instantsearch/deploys/61a624b2036654000854ba7d 😎 Browse the preview: https://deploy-preview-3216--react-instantsearch.netlify.app |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good!
@@ -150,19 +150,19 @@ | |||
}, | |||
{ | |||
"path": "packages/react-instantsearch/dist/umd/Dom.min.js", | |||
"maxSize": "41.50 kB" | |||
"maxSize": "42 kB" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
react 17 increased this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
); | ||
} | ||
|
||
// @TODO: we swallow React act errors in this test because we're unable to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does wrapping in act not work because it's a separate component?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure. I don't understand why it that case it errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok to merge as-is for now I think
This adds the
<DynamicWidgets>
to the Hooks package. It's implemented on top ofuseDynamicWidgets
.<DynamicWidgets>
is a widget that displays matching widgets based on by the corresponding settings of the index.Changes
I upgraded React from 16 to 17 because it better supports Strict Mode and memory leaks warnings, which
<DynamicWidgets>
was prone to. I explain in details how it happens inuseConnector
.Usage