-
Notifications
You must be signed in to change notification settings - Fork 2
feat: new js client #202
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
feat: new js client #202
Conversation
Reviewer's Guide by SourceryThis pull request introduces a new JavaScript client for the SettleMint SDK. The changes include adding new files for the client implementation, schemas, and configuration, as well as updating the build workflow to publish the new JS package. Sequence DiagramsequenceDiagram
participant Client
participant Cache
participant API
Client->>Cache: Check for cached data
alt Data in cache
Cache-->>Client: Return cached data
else Data not in cache
Client->>API: Fetch data
API-->>Client: Return data
Client->>Cache: Store data in cache
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📦 Packages
|
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 @roderik - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider making the cache size and TTL in DEFAULT_OPTIONS configurable for more flexibility.
- Please provide context for the removal of the 'framework' field from ConfigSchema. Are there any implications for existing code?
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟡 Security: 1 issue found
- 🟢 Review instructions: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟡 Documentation: 2 issues found
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| token: ${{ secrets.NPM_TOKEN }} | ||
| package: ./packages/js/package.json | ||
| access: public | ||
| provenance: false |
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.
🚨 suggestion (security): Consider enabling provenance for enhanced package security
Enabling provenance can provide better security guarantees for your published package. If there's a specific reason for disabling it, please document this decision.
| provenance: false | |
| provenance: true |
| @@ -0,0 +1,144 @@ | |||
| import { EthereumAddressSchema, EthereumPublicKeySchema } from "@/schemas/shared"; | |||
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.
suggestion: Consider splitting schemas into separate files for better maintainability
As the number of schemas grows, it might be beneficial to split them into separate files based on their functionality or domain. This could improve readability and make maintenance easier.
import { EthereumAddressSchema, EthereumPublicKeySchema } from "@/schemas/ethereum";
import { UniqueNameSchema, UrlSchema } from "./shared";
import { z } from "zod";
| } | ||
|
|
||
| // biome-ignore lint/suspicious/noExplicitAny: the setters and getters handle the type safety | ||
| const cacheStore: Map<string, CacheEntry<any>> = new Map(); |
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.
suggestion (performance): Consider using a more robust caching solution for production
While the current implementation is a good start, for a production-grade SDK, consider using a well-tested caching library or implementing a more sophisticated caching strategy. This could include features like persistence across sessions or distributed caching for multi-instance deployments.
import NodeCache from 'node-cache';
const cacheStore = new NodeCache({
stdTTL: 600, // 10 minutes default TTL
checkperiod: 120, // Check for expired keys every 2 minutes
useClones: false, // For better performance
});
| <br /> | ||
| </div> | ||
|
|
||
| ## Getting started |
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.
suggestion (documentation): Consider adding content to the 'Getting started' section
This section could include basic installation instructions, a simple usage example, or links to more detailed guides.
## Getting started
### Installation
```bash
npm install @your-package-name
Basic usage
import { someFunction } from '@your-package-name';
// Use the package
someFunction();For more detailed guides, see our documentation.
| <div align="center"> | ||
| <a href="https://console.settlemint.com/documentation/">Documentation</a> | ||
| <span> • </span> | ||
| <a href="https://discord.com/invite/Mt5yqFrey9">Discord</a> |
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.
suggestion (documentation): Consider using a permanent Discord invite link
If possible, it might be beneficial to use a permanent invite link specific to the SettleMint community. This ensures the link won't expire.
| <a href="https://discord.com/invite/Mt5yqFrey9">Discord</a> | |
| <a href="https://discord.gg/settlemint">Discord</a> |
packages/js/src/settlemint.ts
Outdated
| const resources = { | ||
| blockchainNetwork: createResourceHandler<BlockchainNetworkReturnValue>( | ||
| "blockchain-network", | ||
| BlockchainNetworkReturnValueSchema, | ||
| ), | ||
| blockchainNode: createResourceHandler<BlockchainNodeReturnValue>( | ||
| "blockchain-node", | ||
| BlockchainNodeReturnValueSchema, | ||
| ), | ||
| middleware: createResourceHandler<MiddlewareReturnValue>("middleware", MiddlewareReturnValueSchema), | ||
| integrationTool: createResourceHandler<IntegrationToolReturnValue>( | ||
| "integration-tool", | ||
| IntegrationToolReturnValueSchema, | ||
| ), | ||
| storage: createResourceHandler<StorageReturnValue>("storage", StorageReturnValueSchema), | ||
| privateKey: createResourceHandler<PrivateKeyReturnValue>("private-key", PrivateKeyReturnValueSchema), | ||
| insights: createResourceHandler<InsightsReturnValue>("insights", InsightsReturnValueSchema), | ||
| customDeployment: createResourceHandler<CustomDeploymentReturnValue>( | ||
| "custom-deployment", | ||
| CustomDeploymentReturnValueSchema, | ||
| ), | ||
| }; | ||
|
|
||
| return resources; |
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.
suggestion (code-quality): Inline variable that is immediately returned (inline-immediately-returned-variable)
| const resources = { | |
| blockchainNetwork: createResourceHandler<BlockchainNetworkReturnValue>( | |
| "blockchain-network", | |
| BlockchainNetworkReturnValueSchema, | |
| ), | |
| blockchainNode: createResourceHandler<BlockchainNodeReturnValue>( | |
| "blockchain-node", | |
| BlockchainNodeReturnValueSchema, | |
| ), | |
| middleware: createResourceHandler<MiddlewareReturnValue>("middleware", MiddlewareReturnValueSchema), | |
| integrationTool: createResourceHandler<IntegrationToolReturnValue>( | |
| "integration-tool", | |
| IntegrationToolReturnValueSchema, | |
| ), | |
| storage: createResourceHandler<StorageReturnValue>("storage", StorageReturnValueSchema), | |
| privateKey: createResourceHandler<PrivateKeyReturnValue>("private-key", PrivateKeyReturnValueSchema), | |
| insights: createResourceHandler<InsightsReturnValue>("insights", InsightsReturnValueSchema), | |
| customDeployment: createResourceHandler<CustomDeploymentReturnValue>( | |
| "custom-deployment", | |
| CustomDeploymentReturnValueSchema, | |
| ), | |
| }; | |
| return resources; | |
| return { | |
| blockchainNetwork: createResourceHandler<BlockchainNetworkReturnValue>( | |
| "blockchain-network", | |
| BlockchainNetworkReturnValueSchema, | |
| ), | |
| blockchainNode: createResourceHandler<BlockchainNodeReturnValue>( | |
| "blockchain-node", | |
| BlockchainNodeReturnValueSchema, | |
| ), | |
| middleware: createResourceHandler<MiddlewareReturnValue>("middleware", MiddlewareReturnValueSchema), | |
| integrationTool: createResourceHandler<IntegrationToolReturnValue>( | |
| "integration-tool", | |
| IntegrationToolReturnValueSchema, | |
| ), | |
| storage: createResourceHandler<StorageReturnValue>("storage", StorageReturnValueSchema), | |
| privateKey: createResourceHandler<PrivateKeyReturnValue>("private-key", PrivateKeyReturnValueSchema), | |
| insights: createResourceHandler<InsightsReturnValue>("insights", InsightsReturnValueSchema), | |
| customDeployment: createResourceHandler<CustomDeploymentReturnValue>( | |
| "custom-deployment", | |
| CustomDeploymentReturnValueSchema, | |
| ), | |
| }; | |
Explanation
Something that we often see in people's code is assigning to a result variableand then immediately returning it.
Returning the result directly shortens the code and removes an unnecessary
variable, reducing the mental load of reading the function.
Where intermediate variables can be useful is if they then get used as a
parameter or a condition, and the name can act like a comment on what the
variable represents. In the case where you're returning it from a function, the
function name is there to tell you what the result is, so the variable name
is unnecessary.
* main: chore(deps): update dependency @biomejs/biome to v1.9.3 (#203) # Conflicts: # bun.lockb
Summary by Sourcery
Add a new JavaScript client to the SettleMint SDK, enabling integration with SettleMint services. Update the build process to publish the SDK package to npm and enhance the configuration schema by removing an unnecessary field. Include documentation for the new package.
New Features:
Enhancements:
Build:
Documentation: