Refactor client tests, deprecate addProtocolIfNotPresent, fix abort bug, adjust documentation
#1926
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1926 +/- ##
==========================================
- Coverage 99.01% 98.03% -0.98%
==========================================
Files 18 18
Lines 1418 1425 +7
Branches 299 298 -1
==========================================
- Hits 1404 1397 -7
- Misses 14 28 +14 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
WalkthroughThis update refactors internal utility usage, replaces string literals with constants for headers and agent strings, and improves timeout and abort logic. It removes the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant HttpRequests
participant Fetch/HTTP
participant AbortController
Client->>HttpRequests: sendRequest(options)
HttpRequests->>AbortController: create signal (with timeout)
HttpRequests->>Fetch/HTTP: fetch(url, { signal, headers })
alt Timeout occurs
AbortController-->>Fetch/HTTP: abort signal
Fetch/HTTP-->>HttpRequests: throws AbortError
HttpRequests-->>Client: throws MeiliSearchRequestTimeOutError
else Success
Fetch/HTTP-->>HttpRequests: response
HttpRequests-->>Client: response
end
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes detected related to the objectives in the linked issues. Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/http-requests.ts (2)
86-86: Consider replacingvoidwithundefinedin union typeThe static analysis tool correctly flagged that
voidcan be confusing in a union type. Consider usingundefinedinstead for clarity.-): () => (() => void) | void { +): () => (() => void) | undefined {🧰 Tools
🪛 Biome (1.9.4)
[error] 86-86: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
40-40: Fix typo in constant nameThe constant
PACAKGE_AGENTcontains a typo - it should bePACKAGE_AGENT.-const PACAKGE_AGENT = `Meilisearch JavaScript (v${PACKAGE_VERSION})`; +const PACKAGE_AGENT = `Meilisearch JavaScript (v${PACKAGE_VERSION})`;Also update the references to this constant:
- const agents = config.clientAgents.concat(PACAKGE_AGENT); + const agents = config.clientAgents.concat(PACKAGE_AGENT);- headers.set(AGENT_HEADER_KEY, PACAKGE_AGENT); + headers.set(AGENT_HEADER_KEY, PACKAGE_AGENT);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (11)
src/http-requests.ts(7 hunks)src/task.ts(1 hunks)src/types/types.ts(3 hunks)src/utils.ts(1 hunks)tests/client.test.ts(0 hunks)tests/errors.test.ts(0 hunks)tests/health.test.ts(1 hunks)tests/meilisearch.test.ts(1 hunks)tests/network.test.ts(1 hunks)tests/stats.test.ts(1 hunks)tests/version.test.ts(1 hunks)
💤 Files with no reviewable changes (2)
- tests/errors.test.ts
- tests/client.test.ts
🧰 Additional context used
🧬 Code Graph Analysis (4)
tests/health.test.ts (1)
tests/utils/meilisearch-test-utils.ts (2)
getClient(258-258)assert(131-134)
tests/version.test.ts (1)
tests/utils/meilisearch-test-utils.ts (2)
getClient(258-258)assert(131-134)
tests/stats.test.ts (1)
tests/utils/meilisearch-test-utils.ts (2)
getClient(258-258)assert(131-134)
tests/network.test.ts (2)
tests/utils/meilisearch-test-utils.ts (2)
getClient(258-258)assert(131-134)src/types/types.ts (1)
Remote(292-295)
🪛 Biome (1.9.4)
src/http-requests.ts
[error] 86-86: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: integration-tests (Node.js 20)
- GitHub Check: integration-tests (Node.js 22)
- GitHub Check: integration-tests (Node.js 18)
🔇 Additional comments (24)
src/utils.ts (4)
5-5: Good addition for managing warning state.Adding a static boolean flag to track whether the warning has been dispatched is a good approach. This ensures the deprecation warning only appears once per runtime, preventing console spam.
7-9: Nice improvement with regex pattern matching.Replacing multiple string checks with a single regex pattern is more maintainable and concise. The pattern
/^https?:\/\//correctly matches both HTTP and HTTPS protocols.
11-16: Well-implemented deprecation warning.The warning message clearly communicates that the automatic protocol addition feature is being deprecated. The single-dispatch implementation is a good user experience practice, and explicitly showing the host value in the message helps with debugging.
21-21: Proper export list update.The export list has been updated to remove
addTrailingSlashwhich aligns with its removal from the codebase. This ensures the exported API is consistent with the implementation.tests/version.test.ts (2)
1-5: Good test setup with appropriate dependencies.The test properly imports the necessary testing utilities and initializes a client with the required permissions. Using
awaitat the top level is appropriate here for test setup.
6-13: Comprehensive version object validation.This test effectively validates both the structure and types of the version response:
- Verifies the exact number of keys (3)
- Destructures to verify the expected property names exist
- Confirms each property is of type string
Using a loop for the type checks is an efficient approach.
tests/health.test.ts (2)
1-5: Well-structured test setup.The setup follows the same pattern as other test files, promoting consistency across the test suite. The client is properly initialized with master permissions.
6-10: Effective health endpoint validation.The test properly validates both:
- The structure of the health response (exactly one key)
- The expected status value ("available")
This ensures the health endpoint works as intended and maintains its API contract.
src/task.ts (1)
130-130: Important error handling improvement.Adding optional chaining (
?.) when accessing thecauseproperty makes the error handling more robust by safely handling cases where:
- The error might not be an object
- The error might not have a
causepropertyThis prevents potential "Cannot read property 'cause'" errors that would mask the original error.
src/types/types.ts (4)
48-49: Improved documentation clarity.The documentation for the
hostproperty has been simplified to a clear, concise description.
57-59: Grammar correction in documentation.Changed "concatted" to the grammatically correct "concatenated" in the documentation.
67-68: Minor documentation improvement.Changed from "will have to be handled manually" to the more direct "have to be handled manually."
72-79: Enhanced timeout documentation with important implementation details.Excellent addition of detailed documentation for the
timeoutproperty. The comments now explain that it usessetTimeout, which doesn't guarantee precise timing, and includes a link to MDN documentation about potential delays.tests/stats.test.ts (1)
1-37: Well-structured comprehensive test for statistics functionality.The test effectively validates both the structure and type correctness of the response from the
getStatsmethod. It verifies the top-level properties and iterates through each index's statistics to ensure they have the expected structure and types.tests/network.test.ts (1)
17-48: Comprehensive test for network update and retrieval functionality.The test effectively validates the behavior of both
updateNetworkandgetNetworkmethods. It:
- Creates a well-formed network configuration
- Defines a thorough validation function for remote entries
- Verifies the response structure from both methods
- Ensures consistency between the updated and retrieved network configurations
tests/meilisearch.test.ts (4)
20-124: Excellent comprehensive abort and timeout handling tests.This test suite thoroughly examines request abortion scenarios, including:
- Global timeout handling
- Explicit abort signal handling
- Interactions between global timeouts and signal timeouts
- Proper error propagation with expected causes
The tests ensure that the abort controller behavior works correctly in all scenarios, which addresses the bug mentioned in the PR objectives related to incorrect abort controller handling.
126-185: Thorough header composition and priority testing.This test effectively verifies that headers from multiple sources (API key, client agents, global headers, and custom headers) are correctly combined with the expected precedence. It checks specific headers like authorization and client identification headers, as well as the overall header structure.
187-200: Good verification of custom HTTP client functionality.The test confirms that a custom HTTP client function is properly called with the expected parameters, including verifying that the URL passed to the client is correctly constructed based on the configured host.
202-255: Comprehensive error handling tests.This test suite thoroughly covers the three main error types:
MeiliSearchErrorfor configuration errors (invalid host)MeiliSearchRequestErrorfor network-level errorsMeiliSearchApiErrorfor API response errorsEach test verifies that the correct error type is thrown with the expected properties, ensuring robust error handling.
src/http-requests.ts (5)
37-40: LGTM: Good refactoring with constantsReplacing string literals with named constants improves maintainability and reduces the risk of typos. This is a good practice.
84-91: Fixed abort controller bugThis change ensures the correct abort controller signal is passed to the fetch function, addressing the bug mentioned in the PR objectives.
🧰 Tools
🪛 Biome (1.9.4)
[error] 86-86: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
113-115: Improved abort handling with clear timeout identificationThe use of
TIMEOUT_IDas the abort reason makes it easier to identify timeout-specific aborts, improving error handling clarity.Also applies to: 134-136
152-152: Good replacement foraddTrailingSlashThis inline check for trailing slash and conditional addition is a clean replacement for the removed utility function.
16-16:✅ Verification successful
Confirm deprecation implementation of
addProtocolIfNotPresentThis aligns with the PR objective to deprecate automatic protocol addition. Make sure that the implementation of this function in utils.js contains appropriate deprecation warnings.
🏁 Script executed:
#!/bin/bash # Check if addProtocolIfNotPresent has deprecation warnings grep -A 10 "addProtocolIfNotPresent" src/utils.js || grep -A 10 "addProtocolIfNotPresent" src/utils.tsLength of output: 506
Deprecation warning in
addProtocolIfNotPresentconfirmed
The implementation insrc/utils.tsalready logs a one-time deprecation warning viaconsole.warnguarded bywarningDispatched. No further action needed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/http-requests.ts (1)
85-85: Consider usingundefinedinstead ofvoidin union type.The static analysis tool correctly identifies that
voidin union types can be confusing. Consider updating the return type from() => (() => void) | voidto() => (() => void) | undefinedfor better clarity.-): () => (() => void) | void { +): () => (() => void) | undefined {🧰 Tools
🪛 Biome (1.9.4)
[error] 85-85: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/http-requests.ts(7 hunks)src/task.ts(1 hunks)src/types/types.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/task.ts
- src/types/types.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/http-requests.ts (3)
src/package-version.ts (1)
PACKAGE_VERSION(1-1)src/types/types.ts (1)
Config(47-83)src/utils.ts (1)
addProtocolIfNotPresent(21-21)
🪛 Biome (1.9.4)
src/http-requests.ts
[error] 85-85: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: integration-tests (Node.js 20)
- GitHub Check: integration-tests (Node.js 22)
🔇 Additional comments (7)
src/http-requests.ts (7)
16-16: LGTM - Import needed for deprecated function.The import is necessary for the current usage of
addProtocolIfNotPresentin the constructor. Since this function is being deprecated as mentioned in the PR objectives, consider planning for its removal in future versions.
37-40: Excellent refactoring to eliminate magic strings.Replacing string literals with well-named constants improves maintainability and reduces the risk of typos. The constant names are clear and follow consistent naming conventions.
49-63: Consistent application of the new constants.The updates correctly replace string literals with the newly defined constants while preserving the original logic. This enhances code maintainability and consistency.
83-89: Good fix for the abort controller bug.The parameter rename to
initis more concise, and moving the signal assignment earlier (line 89) appears to fix the abort bug mentioned in the PR objectives. This ensures the abort controller signal is properly set before any abort handling logic executes.🧰 Tools
🪛 Biome (1.9.4)
[error] 85-85: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
97-99: Improved timeout and abort handling logic.The refactored timeout handling is cleaner and more consistent. The simplified arrow function and consistent timeout creation pattern enhance code readability.
Also applies to: 112-114, 133-135
148-151: Good inline handling of trailing slash logic.The inline approach to ensure a trailing slash is straightforward and reduces dependencies on utility functions. However, note that
addProtocolIfNotPresentis being deprecated according to the PR objectives, so this usage may need future updates.
181-182: Consistent usage of constants in private method.The update to use
CONTENT_TYPE_KEYinstead of the string literal maintains consistency with the overall refactoring approach throughout the file.
Pull Request
Related issue
Fixes #1922
What does this PR do?
fetchSummary by CodeRabbit
Refactor
Bug Fixes
Tests