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

fix(javascript): order type first in legacy mapping #695

Closed
wants to merge 1 commit into from

Conversation

Haroenv
Copy link
Contributor

@Haroenv Haroenv commented Jun 15, 2022

This is based on an assumption, but if I'd implement this endpoint in the search engine, I would shortcut to the facet search as soon as I see type: facet, so this has a chance to be imperceptually faster

🧭 What and Why

🎟 JIRA Ticket: /

Changes included:

  • type is listed first in type facet request

🧪 Test

Nothing should have changed, except possibly a very small performance improvement

This is based on an assumption, but if I'd implement this endpoint in the search engine, I would shortcut to the facet search as soon as I see `type: facet`, so this has a chance to be imperceptually faster
@Haroenv Haroenv requested a review from shortcuts June 15, 2022 08:52
@netlify
Copy link

netlify bot commented Jun 15, 2022

Deploy Preview for api-clients-automation canceled.

Name Link
🔨 Latest commit 6092a96
🔍 Latest deploy log https://app.netlify.com/sites/api-clients-automation/deploys/62a99db7d4e8230008afa46e

@algolia-bot
Copy link
Collaborator

algolia-bot commented Jun 15, 2022

✗ The generated branch has been deleted.

If the PR has been merged, you can check the generated code on the main branch.
You can still access the code generated on main via this commit.

@@ -3,9 +3,9 @@ if (searchMethodParams && Array.isArray(searchMethodParams)) {
requests: searchMethodParams.map(({ params, ...legacyRequest }) => {
if (legacyRequest.type === 'facet') {
return {
type: 'facet',
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't the spread would put it later in the object (based on where the user defined it)?

Copy link
Member

Choose a reason for hiding this comment

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

maybe we can extract it from the legacyRequest, (would also prevent the TS error in the build)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

destructuring is also an option, but with the current option it will always be the last parameter. I don't think this can make a big difference anyway, I'll leave it up to you how to solve it @shortcuts :)

Copy link
Member

Choose a reason for hiding this comment

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

I'll open an issue from this PR and come back to tackling it when we have more time!

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.

None yet

3 participants