-
Notifications
You must be signed in to change notification settings - Fork 528
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(core): introduce getWidgetRenderState
(2/n)
#4457
feat(core): introduce getWidgetRenderState
(2/n)
#4457
Conversation
This introduces the widget lifecycle hook `getWidgetRenderState` and implements it for `connectSearchBox`.
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 a8d42cd:
|
@@ -569,3 +673,21 @@ const index = (props: IndexProps): Index => { | |||
}; | |||
|
|||
export default index; | |||
|
|||
function storeRenderState({ |
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 think it would be better to have it as a method in index
, it would simplify the signature (no need to pass instantSearchInstance
and parent
).
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.
We call it only twice (and will call it only one time once when remove the init
method). Besides, it would make it available on the index
widget scope, which we want to avoid.
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.
If you do not want to expose it you can set it as a "private" function, like createURL
.
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.
Right, but we still need to pass the instantSearchInstance
reference from init
and render
to mutate it, so it won't behave the same if we rely on localInstantSearchInstance
.
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.
Still winning one parameter 😄 , and it is more coherent with what we are already doing in this module.
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 agree with François, we want to avoid exposing a public API we didn't think about, private isn't really private
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've read all the changes but not confident enough to approve the PR not because of this pr, but because I'm not in a right headspace at the moment. I wish @Haroenv or @yannickcr approve it whenever they think it's ready.
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.
Two questions I'd like to see resolved, otherwise lgtm :)
This introduces the widget lifecycle hook `getWidgetRenderState` and implements it for `connectSearchBox`.
This introduces the widget lifecycle hook `getWidgetRenderState` and implements it for `connectSearchBox`.
* feat(core): introduce `getWidgetUiState` lifecycle hook (1/n) (#4454) This deprecates `getWidgetState` to `getWidgetUiState` to make the upcoming `getWidgetRenderState` less confusing. * feat(core): introduce `getWidgetRenderState` (2/n) (#4457) This introduces the widget lifecycle hook `getWidgetRenderState` and implements it for `connectSearchBox`. * feat(autocomplete): implement `getWidgetRenderState` (#4466) * feat(breadcrumb): implement `getWidgetRenderState` (#4467) * feat(clearRefinements): implement `getWidgetRenderState` (#4468) * feat(configure): implement `getWidgetRenderState` (#4469) * feat(currentRefinements): implement `getWidgetRenderState` (#4470) * fix(breadcrumb): add attribute to render state (#4472) * feat(hierarchicalMenu): implement `getWidgetRenderState` (#4471) * fix: provide both `getWidgetRenderState` and `getRenderState` in connectors (#4518) * WIP * remove temporary implementation * rename getWidgetRenderState to getRenderState * fix wrong parameter name * update panel * update comment * chore: remove panel-related code * fixing types WIP * fix type * do not cast the return of getWidgetRenderState * Revert "do not cast the return of getWidgetRenderState" This reverts commit 614bc53. * Revert "fix type" This reverts commit 54e31fc. * add TWidgetRenderState to connector * add generics to Widget for getWidgetRenderState * fix to allow nullish getWidgetRenderState when unknown * remove exclamation marks * remove the type for widgetParams (was experimental) * make getRenderState optional for widgets with default generics * update other connectors to follow new connector type * update types in tests * add comment * do not declare individual widget render state types * feat(hits): implement `getWidgetRenderState` (#4525) * feat(range): implement `getRenderState` and `getWidgetRenderState` (#4536) * fix(types): fix type errors (#4537) * fix type errors * fix type errors * simplify test util * update type of widgetParams * add lifecycle methods * fixing breadcrumb widget * revert the change (let's do this in later iteration) * add lifecycle methods for places widget * replace WidgetFactory with Factory * remove unnecessary part * rename options to params * Revert "remove unnecessary part" This reverts commit 5b2ed14. * Revert "replace WidgetFactory with Factory" This reverts commit ee125bd. * fix getRenderState of places and analytics * feat(poweredBy): getWidgetRenderState (#4551) * feat(poweredBy): getWidgetRenderState DX-206 * fix jsdoc * feat(menu): implement `getRenderState` and `getWidgetRenderState` (#4540) * feat(menu): implement `getRenderState` and `getWidgetRenderState` * Removed duplicated declaration of `jsHelper`, moved `cachedToggleShowMore` binding to `init` to avoid it happens more than once * feat(renderState): add connectNumericMenu (#4550) * feat(renderState): add connectNumericMenu DX-204 * Apply suggestions from code review Co-authored-by: Yannick Croissant <[email protected]> * Apply suggestions from code review Co-authored-by: Yannick Croissant <[email protected]> * POC: Add telemetry * Update src/widgets/configure-related-items/configure-related-items.ts Co-authored-by: Haroen Viaene <[email protected]> * Use full Schema * POC: Add telemetry 2 * Use middleware * move code a little * chore: fix type of middleware TODO: make sure this stays on rebase Co-authored-by: François Chalifour <[email protected]> Co-authored-by: Eunjae Lee <[email protected]> Co-authored-by: Yannick Croissant <[email protected]> Co-authored-by: Clément Vannicatte <[email protected]> Co-authored-by: Yannick Croissant <[email protected]>
* feat(core): introduce `getWidgetUiState` lifecycle hook (1/n) (#4454) This deprecates `getWidgetState` to `getWidgetUiState` to make the upcoming `getWidgetRenderState` less confusing. * feat(core): introduce `getWidgetRenderState` (2/n) (#4457) This introduces the widget lifecycle hook `getWidgetRenderState` and implements it for `connectSearchBox`. * feat(autocomplete): implement `getWidgetRenderState` (#4466) * feat(breadcrumb): implement `getWidgetRenderState` (#4467) * feat(clearRefinements): implement `getWidgetRenderState` (#4468) * feat(configure): implement `getWidgetRenderState` (#4469) * feat(currentRefinements): implement `getWidgetRenderState` (#4470) * fix(breadcrumb): add attribute to render state (#4472) * feat(hierarchicalMenu): implement `getWidgetRenderState` (#4471) * fix: provide both `getWidgetRenderState` and `getRenderState` in connectors (#4518) * WIP * remove temporary implementation * rename getWidgetRenderState to getRenderState * fix wrong parameter name * update panel * update comment * chore: remove panel-related code * fixing types WIP * fix type * do not cast the return of getWidgetRenderState * Revert "do not cast the return of getWidgetRenderState" This reverts commit 614bc53. * Revert "fix type" This reverts commit 54e31fc. * add TWidgetRenderState to connector * add generics to Widget for getWidgetRenderState * fix to allow nullish getWidgetRenderState when unknown * remove exclamation marks * remove the type for widgetParams (was experimental) * make getRenderState optional for widgets with default generics * update other connectors to follow new connector type * update types in tests * add comment * do not declare individual widget render state types * feat(hits): implement `getWidgetRenderState` (#4525) * feat(range): implement `getRenderState` and `getWidgetRenderState` (#4536) * fix(types): fix type errors (#4537) * fix type errors * fix type errors * simplify test util * update type of widgetParams * add lifecycle methods * fixing breadcrumb widget * revert the change (let's do this in later iteration) * add lifecycle methods for places widget * replace WidgetFactory with Factory * remove unnecessary part * rename options to params * Revert "remove unnecessary part" This reverts commit 5b2ed14. * Revert "replace WidgetFactory with Factory" This reverts commit ee125bd. * fix getRenderState of places and analytics * feat(poweredBy): getWidgetRenderState (#4551) * feat(poweredBy): getWidgetRenderState DX-206 * fix jsdoc * feat(menu): implement `getRenderState` and `getWidgetRenderState` (#4540) * feat(menu): implement `getRenderState` and `getWidgetRenderState` * Removed duplicated declaration of `jsHelper`, moved `cachedToggleShowMore` binding to `init` to avoid it happens more than once * feat(renderState): add connectNumericMenu (#4550) * feat(renderState): add connectNumericMenu DX-204 * Apply suggestions from code review Co-authored-by: Yannick Croissant <[email protected]> * Apply suggestions from code review Co-authored-by: Yannick Croissant <[email protected]> * POC: Add telemetry * Update src/widgets/configure-related-items/configure-related-items.ts Co-authored-by: Haroen Viaene <[email protected]> * Use full Schema * POC: Add telemetry 2 * Use middleware * move code a little * chore: fix type of middleware TODO: make sure this stays on rebase Co-authored-by: François Chalifour <[email protected]> Co-authored-by: Eunjae Lee <[email protected]> Co-authored-by: Yannick Croissant <[email protected]> Co-authored-by: Clément Vannicatte <[email protected]> Co-authored-by: Yannick Croissant <[email protected]>
Related
getWidgetUiState
lifecycle hook (1/n) #4454Description
This introduces the widget lifecycle hook
getWidgetRenderState
and implements it forconnectSearchBox
.The
getWidgetRenderState
method allows to accumulate and store the global render state of the widgets tree. This render state has a similar structure to the UI state but stores the render params.Motivation
When you mount a widget in the InstantSearch tree, you may want to use its render params (e.g.,
refine
function) in another widget to create richer and more interactive interfaces.In InstantSearch.js, it requires using the JavaScript helper, which means that you get out of the InstantSearch API. In React InstantSearch, you can hack around it to use a React context and provide the render params to other components. Still, this is a workaround that goes against React's paradigm.
This PR aims at providing an InstantSearch API layer that provides the render state to all widgets.
Usage
In an InstantSearch widget
The render state is available in the render params.
In a component outside of InstantSearch
The render state is available on the InstantSearch instance on
search.renderState
so that developers can interact with the search state from outside of InstantSearch widgets. This avoids nesting multiple widgets to use their render params. Using widgets also have the cons of relying on the network and the search results to be back before rendering (because we wait for results before rendering). This results in a UI flash, even if you don't want to display only search data coming from the network; this goes against the progressive enhancement paradigm.A pattern that can emerge is to mount virtual widgets to make their render state available on the instance, then use the render state params in another website's element.
The JSX syntax is used for convenience.
With React InstantSearch
The render state could be exposed using a React Context provider:
And using React hooks:
With Vue InstantSearch
This is an example of a
renderState
mixin that exposes the API to Vue InstantSearch.Detailed design
The render state has a similar structure to the UI state but stores the render params.
Storage
The render state is stored on the InstantSearch instance as an object.
API addition
Each widget can now implement a new method called
getWidgetRenderState(indexRenderState, renderOptions): IndexRenderState
.Example
Having an API for the render state (previously called render params) results in a few benefits:
init
andrender
returned render params (they would both usegetWidgetRenderState
)Storing the render state on the InstantSearch instance unlocks APIs that we wanted to implement:
widgetParams
from the InstantSearch instance. This can be useful to track which widget options are used. (E.g.,renderState.instant_search.searchBox.widgetParams
)panel
. We now have thegetWidgetRenderState
API that can be used to compute acanRefine
value, instead of finding the information with the JavaScript helper.How we teach this
Next steps
getWidgetRenderState
method in all widgets