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

refactor(core): move options diff to core #5908

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Haroenv
Copy link
Contributor

@Haroenv Haroenv commented Oct 26, 2023

Summary

We deal with updating of parameters of InstantSearch in both React InstantSearch and Vue InstantSearch, in different ways. This PR aims to fix that, by moving the updating to core.

Result

search.update(props: Partial<Options>)

Still open questions:

  • how to do insights (does middleware need update option too?) (do we handle anything other than init props)
  • how to warn/error for routing/onStateChange (or do we handle it?)
  • do middleware get update function too?
  • test that it works properly of course
  • should some of the code in constructor be moved to update?

Still open questions
- how to do `insights` (does middleware need update option too?) (do we handle anything other than init props)
- how to warn/error for routing/onStateChange (or do we handle it?)
- test that it works properly of course
- should some of the code in constructor be moved to update?
Comment on lines +910 to +927
// TODO: implement update in middleware
// TODO: can this be simplified?
// if (!dequal(this._insights, options.insights)) {
// this._insights = options.insights;
// const existing = this.middleware.find(
// (m) => m.instance.$$type === 'ais.insights' && m.instance.$$internal
// );
// if (options.insights && existing) {
// // TODO: update existing middleware somehow (or maybe remount it)
// } else if (options.insights && !existing) {
// const insightsOptions =
// typeof options.insights === 'boolean' ? {} : options.insights;
// insightsOptions.$$internal = true;
// this.use(createInsightsMiddleware(insightsOptions));
// } else if (!options.insights && existing) {
// this.unuse(existing.creator);
// }
// }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd love for middlewares to remove some of the load from this method. Maybe we could iterate over middlewares, call their update() method, and get a returned value (boolean? object?) which could tell InstantSearch.update() to unuse them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in this case probably existing.update(options.insights) and handling that somehow in the middleware indeed was best, wasn't sure whether the api was worth the coals

Comment on lines +931 to +937
// Updating the `routing` prop is not supported because InstantSearch.js
// doesn't let us change it. This might not be a problem though, because `routing`
// shouldn't need to be dynamic.
// If we find scenarios where `routing` needs to change, we can always expose
// it privately on the InstantSearch instance. Another way would be to
// manually inject the routing middleware in this library, and not rely
// on the provided `routing` prop.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still can't see why routing would need to change during the lifecycle of an InstantSearch app 😅.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah I'd rather ignore or show an error than handling it

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.

2 participants