-
Notifications
You must be signed in to change notification settings - Fork 497
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(helper): set up state management for recommendations #6100
Conversation
export type PlainRecommendParametersWithId = PlainRecommendParameters & { | ||
$$id: string; | ||
}; |
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 added a specific type for recommend parameters augmented with the internal identifier, so we can rely on PlainRecommendParameters
for the queries actually sent to the API.
While not specified in the RFC, I chose here $$id
as the name for the id key, but I'm open to anything else.
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 ea9ca83:
|
@@ -1509,6 +1511,17 @@ AlgoliaSearchHelper.prototype._change = function (event) { | |||
} | |||
}; | |||
|
|||
AlgoliaSearchHelper.prototype._recommendChange = function (event) { |
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.
as we're not sure of the public api, I wonder if it should be _recommend.change
and then we don't really need to be sparse with the number of methods
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'm not sure I get this, could you elaborate? What would we gain in having this?
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.
thinking about it again, not sure if it's changing much, as we don't really need to document/type these "hidden" methods
… RecommendParameters
RecommendationsQuery, | ||
RecommendedForYouQuery, | ||
TrendingQuery, | ||
} from '@algolia/recommend'; |
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.
won't this break if you don't have recommend in the dependency tree? without ts-ignore I think it may cause an error
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.
Oh good call, this could be problematic. In most cases this package would exist through algoliasearch
, but I've added a ts-ignore comment for this import.
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.
It won't exist for existing users that just update InstantSearch but not the client (many people probably).
Also I believe there's some tricky stuff with minification etc that makes the ts-ignore not apply to multiple lines in some cases. Maybe not the case here as this is a hand-written .d.ts, but worth checking in a plain project and comparing with the algoliasearch.d.ts file :)
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 checked and the build script doesn't interact with type files, they are untouched in the package.
2088cf6
to
3613e9a
Compare
Summary
This PR adds the building blocks for handling recommendations in the helper.
Methods for specific models and event handling will be done in subsequent PRs.
Result
The helper can store and manipulate recommend queries.
FX-2765