Add IsValidUrl type #114
Add IsValidUrl type #114
Conversation
WalkthroughThe changes introduce new TypeScript types and tests for handling query strings and validating URLs in the codebase. Key functionalities include extracting query keys, checking for missing or excessive keys, and validating URL structures against defined query parameters. The updates enhance type safety and testing coverage, particularly for the Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 7
🧹 Outside diff range and nitpick comments (5)
src/core/query-string.t-test.ts (3)
10-15: LGTM: ToQueryUnionCase covers essential scenarios.The test cases for ToQueryUnion are comprehensive, covering single parameter, multiple parameters, and repeated parameters scenarios. The expected results are correct based on the input query strings.
Consider adding a test case for an empty query string to ensure it's handled correctly:
Expect<Equal<ToQueryUnion<"">, never>>,
31-41: LGTM: HasExcessiveQueryCase covers essential scenarios.The test cases for HasExcessiveQuery are well-structured and cover important scenarios, including:
- Single properties
- Multiple properties
- Optional properties
- Union types
The expected results are correct based on the input types, ensuring that the HasExcessiveQuery type accurately detects excessive query keys.
Consider adding a test case for an empty object type to ensure it's handled correctly:
Expect<Equal<HasExcessiveQuery<{}, "a">, true>>,
51-70: LGTM: IsValidQueryCase thoroughly tests various scenarios.The test cases for IsValidQuery are comprehensive and cover a wide range of scenarios, including:
- Valid queries
- Missing queries
- Excessive queries
- Combinations of required and optional properties
The expected results correctly include both boolean values and error messages, ensuring that the IsValidQuery type accurately validates queries against object types.
Consider refactoring the error messages to be more specific. Instead of "E: maybe missing query: a", it could be "Error: Missing required query parameter: a". This would make the error messages more informative and easier to understand. Here's an example of how you could modify one of the test cases:
Expect< Equal< IsValidQuery<{ a: string }, "b">, "Error: Missing required query parameter: a" > >,src/fetch/index.t-test.ts (1)
244-248: LGTM with a suggestion: Consider allowing flexible query parameter order.This test case correctly validates that the current implementation throws an error when query parameters are in a different order. However, in real-world scenarios, the order of query parameters typically shouldn't matter.
Consider modifying the implementation to allow for flexible ordering of query parameters, which would make the API more robust and user-friendly.
If you decide to allow flexible ordering, you might want to update this test case to ensure that different orders are accepted:
f(`/api/projects/projectA/workflow/packages/list?a=b&state=true`, { headers: { Cookie: "a=b" }, });src/fetch/index.ts (1)
114-114: Remove unused commented-out codeThe commented-out type
ValidUrlAndQueryappears to be unused. Removing it helps keep the codebase clean and maintainable.Apply this diff to remove the commented-out code:
- // type ValidUrlAndQuery<Query extends string> = Query extends `${string}?${string}`
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- src/core/query-string.t-test.ts (1 hunks)
- src/core/query-string.ts (1 hunks)
- src/fetch/index.t-test.ts (2 hunks)
- src/fetch/index.ts (3 hunks)
🧰 Additional context used
🪛 Biome
src/fetch/index.ts
[error] 31-31: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
🔇 Additional comments (9)
src/core/query-string.t-test.ts (4)
1-8: LGTM: Import statements are correct and necessary.The import statements are well-structured and import all the necessary types for the tests. No unused imports are present.
17-29: LGTM: HasMissingQueryCase thoroughly tests various scenarios.The test cases for HasMissingQuery are comprehensive and cover a wide range of scenarios, including:
- Required and optional properties
- Never type
- Union types
- Various combinations of object types and query keys
The expected results are correct based on the input types, ensuring that the HasMissingQuery type accurately detects missing required keys.
43-49: LGTM: NonOptionalKeysCase effectively tests various scenarios.The test cases for NonOptionalKeys are concise yet comprehensive, covering:
- Objects with only required properties
- Objects with only optional properties
- Objects with a mix of required and optional properties
The expected results are correct based on the input types, ensuring that the NonOptionalKeys type accurately extracts non-optional keys from object types.
1-70: Overall, excellent test coverage for query string utility types.This file provides comprehensive type tests for the query string utility types, covering a wide range of scenarios and edge cases. The test cases are well-structured and ensure robust type checking for:
- ToQueryUnion
- HasMissingQuery
- HasExcessiveQuery
- NonOptionalKeys
- IsValidQuery
The use of
eslint-disablecomments is appropriate to prevent unused variable warnings for these type tests.Some minor suggestions for improvement have been made in previous comments, but these are not critical issues. The overall quality of the tests is high, providing confidence in the correctness of the query string utility types.
src/fetch/index.t-test.ts (2)
232-237: LGTM: Well-structured test case for missing query parameter.This test case effectively validates that an error is thrown when a required query parameter is not provided. It enhances the robustness of the API wrapper by ensuring proper parameter validation.
238-243: LGTM: Comprehensive test case for undefined query parameters.This test case effectively validates that an error is thrown when an undefined query parameter is included in the request. It strengthens the API wrapper by ensuring strict adherence to the defined parameter schema.
src/core/query-string.ts (2)
42-43: Functionality ofExtractQueryis correctThe
ExtractQuerytype correctly extracts the query part from a URL string.
65-67: Implementation ofNonOptionalKeysis correctThe
NonOptionalKeystype accurately extracts non-optional keys from a type.src/fetch/index.ts (1)
104-104: Enhance type safety in 'FetchT' with 'IsValidUrl'Updating the
inputparameter to useIsValidUrlimproves type safety by ensuring that only valid URLs matching the expected query parameters are accepted.
src/fetch/index.t-test.ts
Outdated
| (async () => { | ||
| const f = fetch as FetchT<"", Spec>; | ||
| { | ||
| // TODO: 今はinitの省略ができないが、できるようにしたい | ||
| // methodを省略した場合はgetとして扱う | ||
| const res = await f("/users", {}); | ||
| (await res.json()).prop; | ||
| } | ||
| })(); |
There was a problem hiding this comment.
Remove duplicated code block.
This code block is an exact duplicate of the one immediately preceding it. Removing this redundant segment will improve code maintainability without affecting test coverage.
Please remove the following lines:
- (async () => {
- const f = fetch as FetchT<"", Spec>;
- {
- // TODO: 今はinitの省略ができないが、できるようにしたい
- // methodを省略した場合はgetとして扱う
- const res = await f("/users", {});
- (await res.json()).prop;
- }
- })();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| (async () => { | |
| const f = fetch as FetchT<"", Spec>; | |
| { | |
| // TODO: 今はinitの省略ができないが、できるようにしたい | |
| // methodを省略した場合はgetとして扱う | |
| const res = await f("/users", {}); | |
| (await res.json()).prop; | |
| } | |
| })(); |
| export type IsValidQuery< | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| QueryDef extends Record<string, any>, | ||
| QueryKeys extends string, | ||
| > = [HasMissingQuery<QueryDef, QueryKeys>] extends [true] | ||
| ? `E: maybe missing query: ${keyof QueryDef & string}` | ||
| : [HasExcessiveQuery<QueryDef, QueryKeys>] extends [true] | ||
| ? `E: maybe excessive query: ${QueryKeys}` | ||
| : true; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error messages in IsValidQuery
The current error messages in IsValidQuery may not clearly indicate which query parameters are missing or excessive.
Consider modifying the implementation to specify the exact keys that are missing or excessive to improve developer experience.
Possible approach:
Implement utility types to compute the specific missing or excessive keys:
type MissingKeys<QueryDef, QueryKeys> = Exclude<NonOptionalKeys<QueryDef>, QueryKeys>;
type ExcessiveKeys<QueryDef, QueryKeys> = Exclude<QueryKeys, keyof QueryDef>;Then adjust IsValidQuery to use these types in the error messages:
export type IsValidQuery<
QueryDef extends Record<string, unknown>,
QueryKeys extends string,
> = MissingKeys<QueryDef, QueryKeys> extends never
? ExcessiveKeys<QueryDef, QueryKeys> extends never
? true
: `E: excessive query keys: ${ExcessiveKeys<QueryDef, QueryKeys>}`
: `E: missing query keys: ${MissingKeys<QueryDef, QueryKeys>}`;| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| QueryDef extends Record<string, any>, | ||
| QueryKeys extends string, | ||
| // no union distribution |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using any in type definitions
Using any in type definitions can reduce type safety. Consider replacing any with unknown or a more specific type.
Suggested change:
- QueryDef extends Record<string, any>,
+ QueryDef extends Record<string, unknown>,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | |
| QueryDef extends Record<string, any>, | |
| QueryKeys extends string, | |
| // no union distribution | |
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | |
| QueryDef extends Record<string, unknown>, | |
| QueryKeys extends string, | |
| // no union distribution |
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| QueryDef extends Record<string, any>, | ||
| QueryKeys extends string, | ||
| > = NonOptionalKeys<QueryDef> extends QueryKeys ? false : true; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using any in type definitions
Using any in type definitions reduces type safety. Consider replacing any with unknown or a more specific type to enhance type checking.
Suggested change:
- QueryDef extends Record<string, any>,
+ QueryDef extends Record<string, unknown>,Alternatively, define a more precise type for QueryDef if possible.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | |
| QueryDef extends Record<string, any>, | |
| QueryKeys extends string, | |
| > = NonOptionalKeys<QueryDef> extends QueryKeys ? false : true; | |
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | |
| QueryDef extends Record<string, unknown>, | |
| QueryKeys extends string, | |
| > = NonOptionalKeys<QueryDef> extends QueryKeys ? false : true; |
| export type ToQueryUnion<Query extends string> = | ||
| Query extends `${infer Key}=${string}&${infer Rest}` | ||
| ? Key | ToQueryUnion<Rest> | ||
| : Query extends `${infer Key}=${string}` | ||
| ? Key | ||
| : `invalid query: ${Query}`; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refining the error handling in ToQueryUnion
Currently, if the query string does not match the expected patterns, the ToQueryUnion type returns a string literal of the format invalid query: ${Query}. This may complicate type comparisons and error handling.
Consider returning never or a custom error type to improve type safety and clarity.
Suggested change:
export type ToQueryUnion<Query extends string> =
Query extends `${infer Key}=${string}&${infer Rest}`
? Key | ToQueryUnion<Rest>
: Query extends `${infer Key}=${string}`
? Key
- : `invalid query: ${Query}`;
+ : never;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export type ToQueryUnion<Query extends string> = | |
| Query extends `${infer Key}=${string}&${infer Rest}` | |
| ? Key | ToQueryUnion<Rest> | |
| : Query extends `${infer Key}=${string}` | |
| ? Key | |
| : `invalid query: ${Query}`; | |
| export type ToQueryUnion<Query extends string> = | |
| Query extends `${infer Key}=${string}&${infer Rest}` | |
| ? Key | ToQueryUnion<Rest> | |
| : Query extends `${infer Key}=${string}` | |
| ? Key | |
| : never; |
src/fetch/index.ts
Outdated
|
|
||
| type IsValidUrl< | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| QueryDef extends Record<string, any> | undefined, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using 'any' types and unnecessary ESLint disable comments
Relying on any reduces type safety and can introduce potential bugs. Additionally, disabling ESLint rules suppresses valuable warnings. Consider defining more specific types and removing the ESLint disable comments.
For example:
- Replace
anywith more precise types where possible. - Remove the
eslint-disable-next-linecomments after addressing the underlying issues.
Also applies to: 31-31
| QueryKeys extends string = Query extends string ? ToQueryUnion<Query> : never, | ||
| > = IsValidQuery< | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/ban-types | ||
| QueryDef extends Record<string, any> ? QueryDef : {}, |
There was a problem hiding this comment.
Use 'Record<string, never>' instead of '{}' as a type
Using {} as a type can lead to unintended behaviors since it represents any non-nullable value. For an empty object type, Record<string, never> provides better type safety and clarity.
Apply this diff to fix the issue:
- QueryDef extends Record<string, any> ? QueryDef : {},
+ QueryDef extends Record<string, any> ? QueryDef : Record<string, never>,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| QueryDef extends Record<string, any> ? QueryDef : {}, | |
| QueryDef extends Record<string, any> ? QueryDef : Record<string, never>, |
🧰 Tools
🪛 Biome
[error] 31-31: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/fetch/index.ts (1)
Line range hint
1-110: Summary: Improved URL validation with enhanced type safety.The changes in this file significantly improve type safety for URL handling:
- The new
IsValidUrltype provides a robust mechanism for validating URLs against query definitions.- The updated
FetchTtype now ensures that only valid URLs are accepted as input.These improvements will help catch potential errors at compile-time, reducing the likelihood of runtime issues related to invalid URLs.
To further enhance the code:
- Consider addressing the ESLint disable comments by using more specific types.
- Replace
{}withRecord<string, never>for better type safety when representing empty objects.Overall, these changes represent a positive step towards more robust and type-safe code.
🧰 Tools
🪛 Biome
[error] 31-31: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- src/fetch/index.t-test.ts (1 hunks)
- src/fetch/index.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/fetch/index.t-test.ts
🧰 Additional context used
🪛 Biome
src/fetch/index.ts
[error] 31-31: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
🔇 Additional comments (2)
src/fetch/index.ts (2)
16-18: LGTM: New imports added for URL validation.The new imports (
ExtractQuery,IsValidQuery, andToQueryUnion) are appropriate for implementing theIsValidUrltype. They enhance the type-checking capabilities for URL validation.
102-102: LGTM: Improved type safety for input URL.The update to the
inputparameter type inFetchTis a significant improvement:input: IsValidUrl<Query, Input> extends true ? Input : never,This change ensures that only valid URLs (according to the query definition) are accepted as input. It leverages the new
IsValidUrltype to provide compile-time checks, which will help prevent runtime errors related to invalid URLs.
| type IsValidUrl< | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| QueryDef extends Record<string, unknown> | undefined, | ||
| Url extends string, | ||
| Query extends string | undefined = ExtractQuery<Url>, | ||
| QueryKeys extends string = Query extends string ? ToQueryUnion<Query> : never, | ||
| > = IsValidQuery< | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/ban-types | ||
| QueryDef extends Record<string, any> ? QueryDef : {}, | ||
| QueryKeys | ||
| >; |
There was a problem hiding this comment.
Improve type safety and remove ESLint disable comments.
The IsValidUrl type is a good addition for enhancing URL validation. However, there are a few improvements to be made:
- Remove the ESLint disable comments and address the underlying issues.
- Replace
{}with a more specific type.
Apply this diff to improve type safety:
type IsValidUrl<
- // eslint-disable-next-line @typescript-eslint/no-explicit-any
QueryDef extends Record<string, unknown> | undefined,
Url extends string,
Query extends string | undefined = ExtractQuery<Url>,
QueryKeys extends string = Query extends string ? ToQueryUnion<Query> : never,
> = IsValidQuery<
- // eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/ban-types
- QueryDef extends Record<string, any> ? QueryDef : {},
+ QueryDef extends Record<string, unknown> ? QueryDef : Record<string, never>,
QueryKeys
>;This change removes the need for ESLint disable comments and improves type safety by using Record<string, never> instead of {} for an empty object type.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| type IsValidUrl< | |
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | |
| QueryDef extends Record<string, unknown> | undefined, | |
| Url extends string, | |
| Query extends string | undefined = ExtractQuery<Url>, | |
| QueryKeys extends string = Query extends string ? ToQueryUnion<Query> : never, | |
| > = IsValidQuery< | |
| // eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/ban-types | |
| QueryDef extends Record<string, any> ? QueryDef : {}, | |
| QueryKeys | |
| >; | |
| type IsValidUrl< | |
| QueryDef extends Record<string, unknown> | undefined, | |
| Url extends string, | |
| Query extends string | undefined = ExtractQuery<Url>, | |
| QueryKeys extends string = Query extends string ? ToQueryUnion<Query> : never, | |
| > = IsValidQuery< | |
| QueryDef extends Record<string, unknown> ? QueryDef : Record<string, never>, | |
| QueryKeys | |
| >; |
🧰 Tools
🪛 Biome
[error] 31-31: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
Summary by CodeRabbit
New Features
Bug Fixes
FetchTfunction to ensure valid URLs and query definitions are used.Tests