-
-
Notifications
You must be signed in to change notification settings - Fork 8.6k
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(algolia): upgrade to the new major #10672
base: main
Are you sure you want to change the base?
Conversation
⚡️ Lighthouse report for the deploy preview of this PR
|
✅ [V2]Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎ This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. |
Thanks @millotp Since this is a new version, where can I check the breaking changes that potentially affect us? This seems a bit risky to release this as a minor/patch release.
That's how it affects our internal code. But we also expose some things as part of a public API through themeConfig options: https://docusaurus.io/docs/search#connecting-algolia I'm particularly worried of possible breaking changes to this type that we expose through import type { SearchOptions } from '@algolia/client-search'; Are there any to be aware of? Apart from TS APIs, are there behavior changes to be aware of? For example, different defaults? |
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
@millotp I've seen your Algolia compatibility PR. We could merge this for the next minor, but we could also delay it to the next major. I plan to do a major with a few technical breaking changes (deps/runtime upgrades like Node and TS) so maybe it's a better time to upgrade Algolia as well. Is there a good reason for this to land in v3 and not v4? Does anything regarding the UI or behavior change? If that's the case we may prefer keeping it for v4 and strict compatibility is not a problem. Also, from my perspective I don't really have a reasonable way to verify that the upgrade is retro-compatible 😅 |
Hey @slorber, thanks for the review, here is a brief page about all the breaking changes in v5: https://www.algolia.com/doc/libraries/javascript/v5/upgrade/ Most breaking changes have been handled in DocSearch directly, which exposes a common interface, and
Indeed this type is not fully compatible with the previous one, it's partly fixed by algolia/api-clients-automation#4108 but it's not 100% compatible.
No it's perfectly fine to stay on the client on this version and wait for a next major, my first goal was to upgrade
The only change is a bug fix in docsearch to close the modal with I can try to make the constraint |
Thanks @millotp
Great. As far as I understand you decided for DocSearch it was not worth it to release a new major version and released the Algolia v5 upgrade in DocSearch v3.6.3 patch release right? So, does this mean that you consider it compatible enough to do this safely? (or maybe you explicitly not respect semver?)
If it's not 100% compatible, why did you decide to upgrade Algolia from v4 to v5 in a patch release? That seems risky to me, this means DocSearch users will upgrade to Algolia v5 by just regenerating their lockfile. Is this intended, or do you consider that the differences are so subtle that they shouldn't break any site using DocSearch v3? What are those differences exactly? Note that on our theme, we depend on DocSearch ^3 and Algolia ^4. In practice this means that since you didn't release this upgrade in a new major version, newly initialized Docusaurus sites, and sites upgrading their lockfile, are already using Algolia v5 for the DocSearch modal, but they still use Algolia v4 for the The Algolia dependency is now duplicated, which wasn't the case before. What I mean is, Docusaurus users already use Algolia v5. At best, we could prevent this upgrade by pinning the DocSearch dependency to v3.6.2 (before the upgrade). I assume it's fine to merge this PR because nobody complained about any compatibility issues so far. Just wanted to let you know that it feels a bit risky to do such an upgrade in a patch release 😅 and it also inadvertently increased our site bundle size by duplicating the Algolia dependency in 2 distinct versions.
I'm not sure I understand 🤔 |
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.
Looks good to me, didn't see any regression.
Still curious to have your feedback on a few things
@@ -152,6 +147,7 @@ function DocSearch({ | |||
const closeModal = useCallback(() => { | |||
setIsOpen(false); | |||
searchButtonRef.current?.focus(); | |||
setInitialQuery(undefined); |
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.
Is this what fixes cmd+K?
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.
cmd+k was fixed in docsearch directly, this fixes an issue where the search would always start with the same character if it was already focused once.
packages/docusaurus-theme-search-algolia/src/theme/SearchBar/index.tsx
Outdated
Show resolved
Hide resolved
packages/docusaurus-theme-search-algolia/src/theme/SearchBar/index.tsx
Outdated
Show resolved
Hide resolved
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.
noice
Report too large to display inline |
Hello @slorber, sorry for the delay. While there are many breaking changes in algoliasearch v5, most of them are abstracted in docsearch and the migration should be without any changes for most users. There are some changes to the type like Thank you for the reviews, the changes are pretty minimal now :) |
Pre-flight checklist
Motivation
Algolia has released a new major version of the javascript search client.
Since DocSearch is now using the new version, we can also upgrade the
docusaurus-theme-search-algolia
package to use the latest version of the js client and of DocSearch.There are a few changes to do in the code to accommodate the new version but nothing breaking, it's mostly around types and imports.
Test Plan
The existing tests are enough to test that every works, I also tried the local website and the search is working, with the user agent containing the latest version.
Test links
Deploy preview: https://deploy-preview-10672--docusaurus-2.netlify.app/