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

feat: composition #6437

Merged
merged 29 commits into from
Jan 13, 2025
Merged

feat: composition #6437

merged 29 commits into from
Jan 13, 2025

Conversation

e-krebs
Copy link
Contributor

@e-krebs e-krebs commented Nov 14, 2024

Summary

This PR currently adds support for Composition using a (currently not published yet) Composition API Client:

@e-krebs e-krebs added the 🚨 DO NOT MERGE for a pull request that is ready to review, but should not be merged yet label Nov 14, 2024
@e-krebs e-krebs self-assigned this Nov 14, 2024
@e-krebs e-krebs requested a review from Haroenv November 18, 2024 11:36
@e-krebs e-krebs requested a review from Haroenv November 18, 2024 16:37
Copy link
Contributor

@Haroenv Haroenv left a 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

examples/js/getting-started/src/app.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
packages/algoliasearch-helper/index.d.ts Outdated Show resolved Hide resolved
packages/algoliasearch-helper/src/algoliasearch.helper.js Outdated Show resolved Hide resolved
examples/js/getting-started/package.json Outdated Show resolved Hide resolved
Copy link

codesandbox-ci bot commented Nov 19, 2024

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:

Sandbox Source
example-instantsearch-getting-started Configuration
example-react-instantsearch-getting-started Configuration
example-react-instantsearch-next-app-dir-example Configuration
example-react-instantsearch-next-routing-example Configuration
example-vue-instantsearch-getting-started Configuration

@e-krebs e-krebs force-pushed the feat/composition branch 3 times, most recently from 37c1d4a to 7b6e05b Compare November 19, 2024 11:27
@e-krebs e-krebs requested a review from Haroenv November 19, 2024 11:38
@e-krebs e-krebs marked this pull request as ready for review November 19, 2024 11:38
Copy link
Contributor

@Haroenv Haroenv left a 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)

bundlesize.config.json Outdated Show resolved Hide resolved
@Haroenv Haroenv requested review from a team, dhayab and sarahdayan and removed request for a team November 19, 2024 12:31
@e-krebs e-krebs requested a review from Haroenv November 19, 2024 14:28
@e-krebs
Copy link
Contributor Author

e-krebs commented Nov 20, 2024

@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

Copy link
Contributor

@Haroenv Haroenv left a 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)

packages/algoliasearch-helper/index.d.ts Outdated Show resolved Hide resolved
packages/algoliasearch-helper/index.d.ts Outdated Show resolved Hide resolved
packages/algoliasearch-helper/src/algoliasearch.helper.js Outdated Show resolved Hide resolved
packages/instantsearch.js/src/lib/InstantSearch.ts Outdated Show resolved Hide resolved
packages/instantsearch.js/src/lib/InstantSearch.ts Outdated Show resolved Hide resolved
@Haroenv Haroenv marked this pull request as draft December 9, 2024 09:21
@Haroenv
Copy link
Contributor

Haroenv commented Dec 9, 2024

just as draft as while it's approved, we're not merging until you want a release

e-krebs and others added 11 commits January 2, 2025 15:02
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>
…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
@e-krebs e-krebs mentioned this pull request Jan 6, 2025
@e-krebs e-krebs marked this pull request as ready for review January 8, 2025 10:30
@e-krebs e-krebs requested a review from Haroenv January 8, 2025 10:30
Copy link
Contributor

@Haroenv Haroenv left a 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.

Copy link
Member

@dhayab dhayab left a 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.

@e-krebs
Copy link
Contributor Author

e-krebs commented Jan 13, 2025

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).
Assuming you'll have a metis application I can use to write those examples

@Haroenv Haroenv removed the 🚨 DO NOT MERGE for a pull request that is ready to review, but should not be merged yet label Jan 13, 2025
@Haroenv Haroenv merged commit cee83ab into master Jan 13, 2025
15 checks passed
@Haroenv Haroenv deleted the feat/composition branch January 13, 2025 15:03
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.

3 participants