-
-
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(theme-search-algolia): revert docsearch package range downgrade after bugfix release #9320
Conversation
✅ [V2]
To edit notification comments on pull requests, go to your Netlify site configuration. |
⚡️ Lighthouse report for the deploy preview of this PR
|
@@ -87,6 +87,7 @@ | |||
"@docusaurus/tsconfig": "3.0.0-beta.0", | |||
"@types/jest": "^29.5.3", | |||
"cross-env": "^7.0.3", | |||
"rimraf": "^3.0.2" | |||
"rimraf": "^3.0.2", | |||
"search-insights": "^2.8.2" |
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.
@shortcuts FYI this requires me to add this extra deps to our website because it's an optional peerDeps of docsearch react but it's used for typechecking. Without this extra deps we still get the TypeScript errors:
Error: ../node_modules/@algolia/autocomplete-plugin-algolia-insights/dist/esm/types/InsightsClient.d.ts(1,75): error TS2307: Cannot find module 'search-insights' or its corresponding type declarations.
Error: ../node_modules/@algolia/autocomplete-plugin-algolia-insights/dist/esm/types/InsightsClient.d.ts(2,212): error TS2307: Cannot find module 'search-insights' or its corresponding type declarations.
This is due to our usage of TypeScript option "skipLibCheck": false,
on the website. Most users should use skipLibCheck: true
normally.
Not sure why this deps is an optional peer deps, or if you should do anything, but I thought you might be interested to know ;)
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.
hey @slorber, there's a new optional feature with DocSearch which allow users to track events and we use the search-insights Algolia library to do so, I believe it's optional because the option is opt-in, so I guess the behavior is correct here?
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.
@shortcuts if I use @algolia/autocomplete-plugin-algolia-insights
, isn't it expected that search-insights
should be available in deps? Maybe it's @algolia/autocomplete-plugin-algolia-insights
that should be a peer deps instead?
I don't have a solution to this problem, and not sure to understand it fully, but something feels a bit weird to me in your current setup 😅 Even if your package does not require/import 'search-insights', it still depends on its types, so types should rather be included? I don't know. We probably won't be the only one using skipLibCheck: false
and stumble upon this issue.
Size Change: 0 B Total Size: 1.11 MB ℹ️ View Unchanged
|
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.
We can merge it if you like; we should be adding peer deps ourselves anyway.
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.
thanks!
@@ -87,6 +87,7 @@ | |||
"@docusaurus/tsconfig": "3.0.0-beta.0", | |||
"@types/jest": "^29.5.3", | |||
"cross-env": "^7.0.3", | |||
"rimraf": "^3.0.2" | |||
"rimraf": "^3.0.2", | |||
"search-insights": "^2.8.2" |
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.
hey @slorber, there's a new optional feature with DocSearch which allow users to track events and we use the search-insights Algolia library to do so, I believe it's optional because the option is opt-in, so I guess the behavior is correct here?
Motivation
Follow-up of #9148
Revert downgrade of Algolia DocSearch package after type issue being fixed in v3.5.2 (algolia/docsearch#2007)
Test Plan
CI
Test links
Deploy preview: https://deploy-preview-9320--docusaurus-2.netlify.app/