-
Notifications
You must be signed in to change notification settings - Fork 332
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(umd): batch requests when using plugins via UMD #902
fix(umd): batch requests when using plugins via UMD #902
Conversation
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 e011c0f:
|
In https://app.circleci.com/pipelines/github/algolia/autocomplete/2568/workflows/d4dac733-6ac3-4c56-8ae7-e3a062febf97/jobs/13837 you can see the new test fails with the code on |
packages/autocomplete-preset-algolia/src/requester/createRequester.ts
Outdated
Show resolved
Hide resolved
packages/autocomplete-preset-algolia/src/requester/createRequester.ts
Outdated
Show resolved
Hide resolved
panelContainer.querySelector<HTMLElement>('.aa-Panel') | ||
).toBeInTheDocument(); | ||
|
||
expect(searchClient.search).toHaveBeenCalledTimes(1); |
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.
nice test!
Not necessary now but I'd love to see one more test which I think would have prevented this issue: An E2E test using our UMD builds and counting requests.
Overall, having a test suite counting queries as we make changes would make a lot a sense, given the immediate impact on pricing.
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.
Agree on the end-to-end test. I believe this was initially in the PR then removed?
I recall our end-to-end setup not to be optimal, but this is a good opportunity to fix it given that one underlying issue is how we expect all our builds to work similarly (and in this case, they don't).
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.
there was an UMD E2E test initially indeed, but it wasn't actually easily testing for the case we were fixing. I'll recreate it in a separate PR
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.
For the record, FX-1228 in the backlog is about this.
packages/autocomplete-preset-algolia/src/requester/createRequester.ts
Outdated
Show resolved
Hide resolved
packages/autocomplete-preset-algolia/src/requester/createRequester.ts
Outdated
Show resolved
Hide resolved
packages/autocomplete-preset-algolia/src/requester/createRequester.ts
Outdated
Show resolved
Hide resolved
packages/autocomplete-preset-algolia/src/requester/createRequester.ts
Outdated
Show resolved
Hide resolved
packages/autocomplete-preset-algolia/src/requester/createRequester.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Sarah Dayan <[email protected]>
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.
LGTM as long as the final points can be checked 👍
Thanks @Haroenv for handling this one so promptly!
…ster.ts Co-authored-by: Sarah Dayan <[email protected]>
…en-using-umd-via-script-tags
Pull request was closed
closes FX-1139
fixes #902
If you're using an UMD,
execute
of the algolia requester will not be the same in a plugin as in core, as they each have their own algolia requester. To prevent this, and other requesters accidentally being joined, I introduced a requesterId.I was alternatively thinking of
Symbol.for('aa.algolia-requester')
, but IE11 doesn't support symbols, and it can't be polyfilled