-
Notifications
You must be signed in to change notification settings - Fork 530
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: composition #6437
feat: composition #6437
Conversation
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! I don't think this can be merged as-is, because it would make master non-runnable, but overall this is the way to go
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 a7b5019:
|
37c1d4a
to
7b6e05b
Compare
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.
overall I think this is the way to go, but still think composition id isn't needed in the helper constructor if we assume index name === composition id (because you don't search if you compose instead)
@Haroenv as discussed, this should be ready to merge but we won't merge for now (and instead use this as a base branch for the whole feature implementation) so I'm keeping the "do not merge" label |
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 already cleaner, now just the sortBy to do and moving the state to the index (IMO)
just as draft as while it's approved, we're not merging until you want a release |
Co-authored-by: Haroen Viaene <hello@haroen.me>
Co-authored-by: Haroen Viaene <hello@haroen.me>
feat: forbid adding an index widget when on a composition-based implementation
* feat: make vue compatible with composition * chore: increase vue2 package size 🤏 * chore(lint): allow console.error * feedback: do not add warning
* feat: add support for disjunctive facets with Composition API * fix: no lambda * fix: no string template * Update requestBuilder.js Co-authored-by: Haroen Viaene <hello@haroen.me> --------- Co-authored-by: Haroen Viaene <hello@haroen.me>
* feat: add searchForCompositionFacetValues in the helper * feat: plug searchForFacetValues for Composition * fix: js syntax * fix: bundlesizes * Update packages/algoliasearch-helper/src/algoliasearch.helper.js Co-authored-by: Haroen Viaene <hello@haroen.me> --------- Co-authored-by: Haroen Viaene <hello@haroen.me>
3ce2711
to
0db51c5
Compare
…acetValues (#6506) * example * clean: revert temporary examples change * fix: query & searchQuery where mixed * fix: rebase error
* feat: make composition work server-side * feat: add proper typing for CompositionClient
fix: a small type mistake that went through the cracks
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 seems like the right initial approach for composition.
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 great! I'd suggest adding an example for each flavor once the composition client is released.
@dhayab that's the plan (at least for react & maybe vanilla js too). |
Summary
This PR currently adds support for Composition using a (currently not published yet) Composition API Client:
compositionID
optional parameter when initializing InstantSearch (this PR)