-
Notifications
You must be signed in to change notification settings - Fork 19
Update Node.js Playground SDK and improve reliability #30
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
base: master
Are you sure you want to change the base?
Conversation
…ling - Update node-appwrite dependency from 14.0.0 to 21.1.0 - Add Runtime and ExecutionMethod imports for future use - Add deploymentId and variableId tracking variables - Improve uploadDeployment to poll for deployment status instead of fixed wait - Ensure deployment is ready before executing functions
WalkthroughThe pull request updates the Appwrite SDK dependency from version 14.0.0 to ^21.1.0 in package.json. In src/app.js, the changes add Runtime and ExecutionMethod to the SDK imports, introduce two new top-level variables (deploymentId and variableId), and implement a polling mechanism for deployment status checking. The new polling loop replaces a fixed wait by repeatedly calling getDeployment until the status reaches 'ready' or 'failed', with logging at each poll iteration and timeout protection. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Comment |
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.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app.js (1)
420-440: Fix incompatible SDK v21 API calls—all use outdated positional parameters.The installed SDK (node-appwrite v21.1.0) requires named parameters, but the code uses positional parameters. This affects
functions.create(),functions.createDeployment(), andfunctions.createExecution()throughout the file. For example,functions.create()must be rewritten from:functions.create(ID.unique(), "Node Hello World", "node-16.0", [Role.any()], ...)to:
functions.create({ functionId: ID.unique(), name: "Node Hello World", runtime: sdk.Runtime.Node145, execute: ["any"], ... })Also update
"node-16.0"to use the runtime enum (e.g.,sdk.Runtime.Node145) and change[Role.any()]to["any"].
🧹 Nitpick comments (2)
src/app.js (2)
1-1:RuntimeandExecutionMethodare imported but not used.These enums are imported but the code still uses string literals:
- Line 426:
"node-16.0"instead ofRuntime.Node160- Line 506:
"GET"instead ofExecutionMethod.GetConsider using the imported enums for type safety, or remove them if intentionally keeping string literals.
♻️ Optional: Use the imported enums
const createFunction = async () => { console.log(chalk.greenBright('Running Create Function API')); const response = await functions.create( ID.unique(), "Node Hello World", - "node-16.0", + Runtime.Node160, [Role.any()],const executeSync = async () => { console.log(chalk.greenBright('Running Execute Function API (sync)')); - let response = await functions.createExecution(functionId, "", false, "/", "GET", {}); + let response = await functions.createExecution(functionId, "", false, "/", ExecutionMethod.Get, {});
28-29:variableIdis declared but never used.
deploymentIdis correctly used in the new polling logic, butvariableIdhas no references in this file. If it's intended for future use, consider adding it when needed; otherwise, remove it to avoid dead code.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
package.jsonsrc/app.js
🔇 Additional comments (2)
src/app.js (1)
468-492: Good improvement: Polling replaces fixed wait.The polling mechanism with status checks is a solid improvement over a fixed delay. The timeout (60s = 30 attempts × 2s) and failure handling are appropriate.
One consideration: unexpected status values (neither 'ready' nor 'failed') will silently loop until timeout. For a playground this is acceptable, but you could optionally log a warning for unrecognized statuses.
💡 Optional: Log unexpected statuses
if (deployment.status === 'ready') { console.log("Deployment is ready!"); break; } else if (deployment.status === 'failed') { throw new Error("Deployment failed to build"); + } else if (!['building', 'processing', 'pending'].includes(deployment.status)) { + console.log(`Unexpected deployment status: ${deployment.status}`); }package.json (1)
13-13: Major SDK version update from 14.x to 21.x requires compatibility verification.This is a significant version jump spanning 7 major versions (14 to 21). The code changes in
src/app.jsconfirm the API updates have been accommodated with the newRuntimeandExecutionMethodimports. Verify that all SDK method signatures in the application are compatible with v21.1.0 and review the changelog for any behavioral differences in the updated SDK.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Summary
This PR updates the Node.js playground to use the latest node-appwrite SDK and improves the reliability of function deployment and execution.
Changes
Dependencies
Updated node-appwrite from 14.0.0 to ^21.1.0
Code Improvements
Summary by CodeRabbit
Release Notes
Dependencies
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.