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

chore: export AlgoliaAnalytics #252

Closed
wants to merge 3 commits into from
Closed

chore: export AlgoliaAnalytics #252

wants to merge 3 commits into from

Conversation

eunjae-lee
Copy link
Contributor

@eunjae-lee eunjae-lee commented Apr 1, 2021

This PR exports AlgoliaAnalytics to create insights client for those who want more control.
Before, it was only possible to choose which requester to use.

This PR enables algolia/instantsearch#4712

@codesandbox-ci
Copy link

codesandbox-ci bot commented Apr 1, 2021

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 86b8fc5:

Sandbox Source
Vanilla Configuration

@eunjae-lee eunjae-lee changed the title test WIP fix: export functions to create insights client Apr 1, 2021
@eunjae-lee eunjae-lee marked this pull request as ready for review April 1, 2021 14:10
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.

reasonable, but not sure if it's needed for the algolia/instantsearch#4712 PR since createInsightsClient already is exposed

export { getRequesterForBrowser, getRequesterForNode };
export {
getRequesterForBrowser,
getRequesterForNode,
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand why browser exports the node requester, but that seems to already have been the case?

Copy link
Contributor Author

@eunjae-lee eunjae-lee Apr 6, 2021

Choose a reason for hiding this comment

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

Clearly it doesn't need to export it. It was there by mistake. I'll get rid of it in the v2.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it still tree shaked off by builder?

@eunjae-lee
Copy link
Contributor Author

reasonable, but not sure if it's needed for the algolia/instantsearch.js#4712 PR since createInsightsClient already is exposed

https://github.com/algolia/instantsearch.js/pull/4712/files#diff-02b3ab8202d2644b709760f62e00f44461af131e42f694fec1f6388ef080af03R2-R5
It needs to use AlgoliaAnalytics, getFunctionalInterface, getRequesterForBrowser, processQueue.

@eunjae-lee eunjae-lee changed the title fix: export functions to create insights client chore: export AlgoliaAnalytics Apr 6, 2021
@eunjae-lee
Copy link
Contributor Author

This PR has been changed from

"Exporting AlgoliaAnalytics, getFunctionalInterface, getRequesterForBrowser, processQueue"

to

"Exporting AlgoliaAnalytics".

@Haroenv
Copy link
Contributor

Haroenv commented Apr 7, 2021

What about importing from /insights in the InstantSearch test?

@eunjae-lee
Copy link
Contributor Author

What about importing from /insights in the InstantSearch test?

good idea! then we can just close this PR.

algolia/instantsearch@70cde71

@eunjae-lee
Copy link
Contributor Author

eunjae-lee commented Apr 8, 2021

Closing this PR since we'll directly import the functions from the source in algolia/instantsearch@70cde71

(It's only used in the test cases in InstantSearch.js. If it needs to be properly exported for other usecases, then we will reconsider this PR.)

@eunjae-lee eunjae-lee closed this Apr 8, 2021
@eunjae-lee eunjae-lee deleted the chore/wip branch April 8, 2021 08:22
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