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

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open

feat: composition #6437

wants to merge 18 commits into from

Conversation

e-krebs
Copy link

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

Summary

This PR currently adds two things:

  • new compositionID optional parameter when initializing InstantSearch
    • instantsearch({ compositionID: 'composition-2', searchClient });
  • new searchWithComposition method in AlgoliaSearchHelper to handle querying the composition API
  • also, since we have to use a root index component (automagically added) under the hood, I had to adapt the AlgoliaSearchHelper signature to handle being initialized with an indexName vs. a compositionID
    • I made it so this is backward compatible
    • maybe there's a better way?
    • we're using the index parameter internally to store the compositionID. The main reason for that is: this is the simplest way to be out of the box compatible with the sortBy widget

Result

I'm currently modifying the js > getting-started example.
A dedicated example will be added later.

@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 9723a37:

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 2 times, most recently from 29edc49 to 37c1d4a Compare November 19, 2024 11:17
@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
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🚨 DO NOT MERGE for a pull request that is ready to review, but should not be merged yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants