fix(types): moved all types to separated folder for better maintainability of the client#440
Conversation
…ility of the client
|
Warning Rate limit exceeded@gkorland has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 32 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
WalkthroughRefactors and centralizes many scattered type and enum declarations into new Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as FalkorDB.connect
participant Pool as connection pool
participant Client as Redis client (single/cluster/sentinel)
participant Factory as clientFactory
participant Wrapper as FalkorDB instance
Note over Caller,Factory: Connect flow (new routing)
Caller->>Pool: request underlying client
Pool->>Client: create/obtain client
Client->>Factory: provide client + serverInfo
alt server is sentinel
Factory->>Wrapper: create Sentinel wrapper
else server is cluster
Factory->>Wrapper: create Cluster wrapper
else single
Factory->>Wrapper: create Single wrapper
end
Wrapper-->>Caller: return initialized FalkorDB
note right of Wrapper: wrapper wires commands & events
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Areas requiring extra attention:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
src/clients/sentinel.ts (1)
2-7: Consider using type-only imports for the shared connection types.
SingleGraphConnectionandTypedRedisClientOptionsare used only in type positions here; consider importing them withimport type { ... } from "../types";to make the intent explicit and keep this module’s runtime dependencies minimal. Runtime imports forSingleandFalkorDBlook correct.src/graph.spec.ts (1)
8-151: Consider reducing casting repetition.The pattern
client as Clientis repeated throughout all test cases. While functionally correct, you could reduce repetition by either:
- Updating the test utility type signature to return
Clientdirectly, or- Creating a helper function that wraps the casting
This is a minor readability improvement and can be deferred if preferred.
src/clients/cluster.ts (1)
10-10: Remove the unnecessary empty named import.The empty
{ }in the import statement serves no purpose and can be cleaned up.-import FalkorDB, { } from "../falkordb"; +import FalkorDB from "../falkordb";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (38)
index.ts(1 hunks)src/clients/client.ts(1 hunks)src/clients/cluster.ts(1 hunks)src/clients/nullClient.ts(1 hunks)src/clients/sentinel.ts(1 hunks)src/clients/single.ts(1 hunks)src/commands/CONFIG_GET.ts(1 hunks)src/commands/CONSTRAINT_CREATE.ts(1 hunks)src/commands/CONSTRAINT_DROP.ts(1 hunks)src/commands/MEMORY_USAGE.ts(1 hunks)src/commands/QUERY.ts(1 hunks)src/commands/SLOWLOG.ts(1 hunks)src/commands/index.ts(1 hunks)src/enums/constraint-type.ts(1 hunks)src/enums/entity-type.ts(1 hunks)src/enums/graph-value-types.ts(1 hunks)src/enums/index.ts(1 hunks)src/falkordb.ts(1 hunks)src/graph.spec.ts(12 hunks)src/graph.ts(1 hunks)src/test-utils.ts(1 hunks)src/types/cluster-graph-connection.ts(1 hunks)src/types/config-item.ts(1 hunks)src/types/falkordb-options.ts(1 hunks)src/types/graph-entity-raw-properties.ts(1 hunks)src/types/graph-metadata.ts(1 hunks)src/types/graph-raw-value.ts(1 hunks)src/types/index.ts(1 hunks)src/types/memory-usage-options.ts(1 hunks)src/types/memory-usage-reply.ts(1 hunks)src/types/net-socket-options.ts(1 hunks)src/types/query-options.ts(1 hunks)src/types/query-reply.ts(1 hunks)src/types/single-graph-connection.ts(1 hunks)src/types/slow-log.ts(1 hunks)src/types/socket-options.ts(1 hunks)src/types/tls-socket-options.ts(1 hunks)src/types/typed-redis-client-options.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-07-28T13:01:44.240Z
Learnt from: gkorland
Repo: FalkorDB/falkordb-ts PR: 0
File: :0-0
Timestamp: 2024-07-28T13:01:44.240Z
Learning: The correct URL format for authentication in Redis and FalkorDB includes credentials before the protocol, as in `redis://user:passlocalhost:6379` and `falkor://user:passlocalhost:6379`.
Applied to files:
src/types/falkordb-options.ts
🧬 Code graph analysis (6)
src/types/graph-entity-raw-properties.ts (1)
src/types/graph-raw-value.ts (1)
GraphRawValue(34-50)
src/enums/constraint-type.ts (1)
index.ts (1)
ConstraintType(2-2)
src/types/socket-options.ts (2)
src/types/net-socket-options.ts (1)
NetSocketOptions(3-5)src/types/tls-socket-options.ts (1)
TlsSocketOptions(3-5)
src/types/graph-raw-value.ts (3)
src/graph.ts (5)
id(267-297)id(299-318)T(145-150)T(153-161)T(163-185)src/types/graph-entity-raw-properties.ts (1)
GraphEntityRawProperties(3-3)src/types/query-reply.ts (1)
QueryReply(16-24)
src/enums/entity-type.ts (1)
index.ts (1)
EntityType(2-2)
src/graph.spec.ts (2)
src/graph.ts (1)
Graph(25-502)src/clients/client.ts (1)
Client(7-84)
🔇 Additional comments (36)
src/commands/CONSTRAINT_DROP.ts (1)
1-1: LGTM! Clean refactoring to centralized enums.The import path update correctly references the new centralized enums module, and the function signatures remain unchanged.
src/types/memory-usage-options.ts (1)
1-3: LGTM! Clean type definition.The interface is well-defined with an appropriately typed optional property.
src/types/memory-usage-reply.ts (1)
1-1: LGTM! Well-defined recursive type.The recursive type definition correctly models nested array structures containing strings, numbers, or further nested arrays.
src/types/single-graph-connection.ts (1)
1-8: LGTM! Well-structured type definition.The type alias correctly extends
RedisClientTypewith a typedfalkordbcommand namespace, providing strong typing for the FalkorDB client connection.src/enums/graph-value-types.ts (1)
1-20: LGTM! Well-documented enum with upstream reference.The enum is clearly defined with a helpful comment linking to the upstream source. This makes it easy to verify correctness and track changes in the FalkorDB implementation.
src/commands/MEMORY_USAGE.ts (1)
1-1: LGTM! Clean migration to centralized types.The import correctly references the newly centralized type definitions, and the function implementations remain unchanged.
src/enums/entity-type.ts (1)
1-4: LGTM! Clean enum definition.The string-valued enum correctly models the two entity types used in FalkorDB constraint operations.
src/test-utils.ts (1)
1-2: Manual verification required—automated checks unavailable.The import has changed from the external package
@redis/test-utilsto a local module./test-utils/lib. Verify that:
- The local module exists at
src/test-utils/lib- It exports the same
TestUtilsinterface as the previous external packagesrc/types/config-item.ts (1)
2-5: ConfigItem centralization matches existing usage.Exposing
ConfigItemas a shared[configKey: string, value: number]tuple type is consistent with its described use inCONFIG_GETand improves reuse without changing behavior.src/enums/constraint-type.ts (1)
1-4: ConstraintType enum extraction is clean and reusable.Defining
ConstraintTypeonce insrc/enumsand reusing it across commands/graph keeps the constraint surface consistent and easier to maintain.src/commands/CONFIG_GET.ts (1)
1-9: Using shared ConfigItem from ../types keeps the API stable.Switching to
ConfigItemfrom the types barrel keeps thetransformReplysignature intact while centralizing the definition; no runtime impact.index.ts (1)
2-2: Re-exporting enums from CONSTRAINT_CREATE preserves public surface.Pointing
ConstraintTypeandEntityTypeat./src/commands/CONSTRAINT_CREATEis fine as long as that module exports them; external consumers of the rootindex.tsremain unaffected.src/types/graph-metadata.ts (1)
1-5: GraphMetadata interface is straightforward and well-named.Centralizing
labels,relationshipTypes, andpropertyKeysunderGraphMetadatamakes graph metadata usage clearer across the codebase without affecting runtime behavior.src/types/tls-socket-options.ts (1)
1-5: TlsSocketOptions discriminant fits the socket options design.Extending
tls.ConnectionOptionsand addingtls: truegives a clear discriminator for TLS sockets, which pairs well with the non-TLS side (NetSocketOptionswithtls?: false) for safer narrowing in callers.src/types/net-socket-options.ts (1)
1-5: NetSocketOptions pairs cleanly with TlsSocketOptions.Modeling plain sockets as
Partial<net.SocketConnectOpts> & { tls?: false; }gives a symmetric discriminant withTlsSocketOptions, making the combined socket options type easier to reason about and safer to consume.src/graph.spec.ts (1)
3-4: LGTM! Clean import refactoring.The removal of
GraphClientTypeand direct use of theClientinterface aligns well with the centralized type structure.src/types/slow-log.ts (1)
1-13: LGTM! Clean type definitions.The type definitions clearly distinguish between the raw reply format (string-based) and the transformed reply format (Date and number types). This provides good type safety for slow log operations.
src/clients/nullClient.ts (1)
4-5: LGTM! Imports successfully consolidated.The import statements now correctly reference the centralized
enumsandtypesmodules, improving maintainability without affecting the NullClient's functionality.src/enums/index.ts (1)
1-3: LGTM! Standard barrel export pattern.This centralized export point for enums follows best practices and makes imports cleaner throughout the codebase.
src/types/query-options.ts (1)
2-13: LGTM! Well-structured query option types.The recursive type definition for
QueryParamcorrectly handles nested parameter structures, andQueryOptionsBackwardCompatibleprovides backward compatibility for code that might pass a simple timeout number.src/types/query-reply.ts (1)
1-24: LGTM! Well-designed discriminated union types.The type definitions correctly model two query scenarios (with and without result data) using discriminated unions. The tuple and object variants provide flexibility for different use cases.
src/types/graph-entity-raw-properties.ts (1)
1-3: LGTM! Clean type definition.The
GraphEntityRawPropertiestype alias correctly models graph entity properties as tuples with an ID and variable-length values. The dependency onGraphRawValueis appropriately imported.src/clients/client.ts (1)
3-4: LGTM! Imports successfully consolidated.The import statements now correctly reference the centralized
enumsandtypesmodules, consistent with the refactoring across the codebase. TheClientinterface remains unchanged.src/commands/CONSTRAINT_CREATE.ts (1)
1-15: Enum import refactor maintains behaviorSwitching
ConstraintTypeandEntityTypeto come from the shared../enumsbarrel keepstransformArgumentsand the emitted argument list unchanged while improving reuse. No issues spotted.src/types/cluster-graph-connection.ts (1)
1-8: ClusterGraphConnection typing matches command namespace
RedisClusterType<{ falkordb: typeof commands }, RedisFunctions, RedisScripts>correctly reflects the command module shape and provides a strongly typed cluster connection without altering runtime behavior.src/commands/index.ts (1)
18-24: Centralizing query types via../typeslooks goodImporting
QueryOptionsBackwardCompatible,QueryParam, andQueryParamsfrom the shared types module keepspushQueryArgumentsand helpers’ behavior intact while reducing duplication.src/types/falkordb-options.ts (1)
4-56: FalkorDBOptions structure and docs look consistentThe
FalkorDBOptionsinterface cleanly aggregates connection, auth, client-name, heartbeat, and pool settings and reuses the centralizedSocketOptions/PoolOptionstypes. No issues detected.src/commands/QUERY.ts (1)
2-4: Query types correctly moved to the shared types moduleImporting
QueryOptionsBackwardCompatible,QueryRawReply, andQueryReplyfrom../typeskeeps bothtransformArgumentsandtransformReplybehavior identical while centralizing the type surface.src/commands/SLOWLOG.ts (1)
1-18: SlowLog types import aligns with transformReply mappingUsing
SlowLogRawReply/SlowLogReplyfrom the shared../typesmodule matches the[timestamp, command, query, took]tuple structure used intransformReply. No behavioral change introduced.src/clients/single.ts (1)
6-7: LGTM!The import consolidation correctly moves enum and type imports to the centralized modules. All imported types (
ConstraintType,EntityType,MemoryUsageOptions,MemoryUsageReply,QueryOptions,SingleGraphConnection) are appropriately used throughout the class.src/graph.ts (1)
3-19: LGTM!The refactored imports correctly consolidate all graph-related types and enums from the centralized modules. All imported symbols are actively used throughout the class for parsing graph data structures, metadata handling, and query operations.
src/falkordb.ts (1)
12-13: LGTM on the new centralized imports.The imports for
FalkorDBOptions,TypedRedisClientOptions, and the defaultcommandsmodule are correctly sourced from the new centralized locations.src/clients/cluster.ts (1)
13-21: LGTM!The centralized imports for types and enums are correctly structured. All imported symbols are properly utilized throughout the
Clusterclass implementation.src/types/index.ts (1)
1-16: LGTM!This barrel file correctly consolidates all type exports into a single entry point, improving maintainability and simplifying imports throughout the codebase. The
export *pattern is appropriate for type re-exports.src/types/graph-raw-value.ts (2)
6-50: LGTM on the raw value type definitions.The discriminated union types for
GraphRawValueand its components (GraphMapRawValue,GraphPathRawValue,GraphEdgeRawValue,GraphNodeRawValue) correctly model the wire format withGraphValueTypesdiscriminators. The structure aligns with the parsing logic insrc/graph.ts.
52-99: Type definitions are well-structured.The parsed value types (
GraphEntityProperties,GraphEdge,GraphNode,GraphPath,GraphMap,GraphValue,GraphReply<T>) correctly represent the domain model. TheGraphReply<T>generic properly extendsQueryReplywhile replacing the data type.
| | { | ||
| latitude: string; | ||
| longitude: string; | ||
| } |
There was a problem hiding this comment.
Point type should use number for coordinates, not string.
In src/graph.ts (lines 232-236), the #parseValue method converts the raw string coordinates to numbers using parseFloat. The GraphValue union should reflect the parsed type, not the raw wire format.
| {
- latitude: string;
- longitude: string;
+ latitude: number;
+ longitude: number;
}📝 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.
| | { | |
| latitude: string; | |
| longitude: string; | |
| } | |
| | { | |
| latitude: number; | |
| longitude: number; | |
| } |
🤖 Prompt for AI Agents
In src/types/graph-raw-value.ts around lines 87 to 90, the Point type currently
declares latitude and longitude as strings but the parsed values are numbers;
update the Point type to use number for latitude and longitude and update any
related GraphValue union typings to reflect that Point now carries numeric
coordinates so TypeScript matches the parsed output from #parseValue; then run
type checks and update any usages that assumed string coordinates.
2 participants
Since i noticed that multiple types and enums were used directly which is making maintainability of the client hard, right now types and enums are in separated folders so we can use them correctly in the whole application without redundant imports.
Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.