Skip to content

Fix: node to use chunking for uploads#1291

Merged
ChiragAgg5k merged 7 commits intomasterfrom
fix-node-sync
Apr 20, 2026
Merged

Fix: node to use chunking for uploads#1291
ChiragAgg5k merged 7 commits intomasterfrom
fix-node-sync

Conversation

@ItzNotABug
Copy link
Copy Markdown
Contributor

@ItzNotABug ItzNotABug commented Jan 10, 2026

What does this PR do?

This updates the generated Node SDK upload path so file parameters can be passed as either the standard File type or the SDK-provided InputFile helper.

Before this change, InputFile.fromPath() eagerly read the whole file into memory before the request was sent. That made large uploads expensive and bypassed the client's chunked upload flow. This PR keeps path-backed files as lazy InputFile sources, lets the Node client detect them in chunkedUpload(), and reads only the active chunk when a file is larger than the configured chunk size.

Implementation details

  • Adds Node-specific file parameter typing so generated upload methods accept File | InputFile.
  • Handles future array-of-file parameters as (File | InputFile)[] instead of binding only InputFile[] to the array suffix.
  • Adds a hasFileParam Twig filter so generated service files import InputFile only when the service has file parameters.
  • Reworks templates/node/src/inputFile.ts.twig so InputFile can be backed by:
    • a path, read lazily through fs only when size/slice/materialization is needed;
    • a Uint8Array, ArrayBuffer, or string;
    • a Blob-like object with arrayBuffer() and slice().
  • Updates Client.chunkedUpload() to handle both File and InputFile inputs.
  • Preserves current master JSON bigint handling in the Node client while resolving merge conflicts.
  • Avoids top-level fs/path imports in InputFile, so importing the helper does not immediately break edge-style runtimes. Path-based filesystem methods now throw a clear runtime error when filesystem access is unavailable.
  • Updates Node 16/18/20 expected output for the large upload cases.

Example usage

Path-backed uploads now stay lazy and are chunked without loading the full file into memory:

import { Client, Storage, ID } from 'node-appwrite';
import { InputFile } from 'node-appwrite/file';

const client = new Client()
  .setEndpoint('https://cloud.appwrite.io/v1')
  .setProject('<PROJECT_ID>')
  .setKey('<API_KEY>');

const storage = new Storage(client);

await storage.createFile({
  bucketId: '<BUCKET_ID>',
  fileId: ID.unique(),
  file: InputFile.fromPath('./large-video.mp4', 'large-video.mp4'),
  onProgress: (progress) => {
    console.log(`${progress.chunksUploaded}/${progress.chunksTotal}`, progress.progress);
  },
});

Buffer, ArrayBuffer, Uint8Array, string, and Blob-like inputs are also supported:

import { InputFile } from 'node-appwrite/file';

const fromBuffer = InputFile.fromBuffer(await fs.promises.readFile('./avatar.png'), 'avatar.png');
const fromPlainText = InputFile.fromPlainText('hello world', 'hello.txt');
const fromArrayBuffer = InputFile.fromBuffer(await fetch(url).then((res) => res.arrayBuffer()), 'remote.bin');

Existing native File inputs continue to work:

import { File } from 'node-fetch-native-with-agent';

await storage.createFile({
  bucketId: '<BUCKET_ID>',
  fileId: ID.unique(),
  file: new File([new Uint8Array([1, 2, 3])], 'bytes.bin'),
});

Edge-style runtimes can import and use non-filesystem helpers. Filesystem-backed helpers fail only when they are actually used:

const file = InputFile.fromPlainText('edge safe', 'note.txt'); // Works

InputFile.fromPath('./local.txt');
// Throws: File system operations are not supported in edge runtimes. Please use InputFile.fromBuffer instead.

Conflict resolution

This branch was refreshed against current master (d280f89c3). The conflicts were in:

  • src/SDK/Language/Node.php
  • templates/node/src/client.ts.twig

The Node type override was reduced to only customize file parameters and delegate all other type resolution to Web, so current master type behavior such as int64 and model handling is preserved.

Test Plan

  • docker run --rm -v $(pwd):/app -w /app php:8.3-cli php example.php node
  • php vendor/bin/phpunit --filter Node20Test
  • node -e "globalThis.EdgeRuntime='edge'; const { InputFile } = require('./tests/sdks/node/dist/inputFile.js'); Promise.resolve().then(async () => { const file = InputFile.fromPlainText('hello', 'hello.txt'); const size = await file.size(); if (size !== 5) throw new Error('unexpected size ' + size); try { InputFile.fromPath('/tmp/file.txt'); throw new Error('fromPath did not throw'); } catch (error) { if (!String(error.message).includes('edge runtimes')) throw error; } console.log('edge inputfile smoke:passed'); }).catch((error) => { console.error(error); process.exit(1); });"
  • composer lint-twig
  • php -l src/SDK/Language/Node.php
  • git diff --check

Related PRs and Issues

Have you read the Contributing Guidelines on issues?

Yes.

@ItzNotABug ItzNotABug self-assigned this Jan 10, 2026
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Jan 10, 2026

Walkthrough

Node language type for file parameters changed from File to File | InputFile. A new hasFileParam helper and getFilters were added to the Node SDK language class. The generated Node client now imports and supports InputFile when services have file parameters. InputFile was reworked into a self-contained class with factories (fromBuffer, fromPath, fromPlainText), a filename property, and methods size(), slice(), and toFile() backed by multiple internal source types. Client chunked upload logic was updated to handle both File and InputFile (single vs chunked uploads, slicing, progress events, per-chunk response IDs). Tests updated to expect multiple large-file upload responses.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Fix: node to use chunking for uploads' accurately summarizes the main change: implementing chunked uploads for the Node implementation, which aligns with the PR objectives and the primary modifications across all files.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0cd6e8b and b34696e.

📒 Files selected for processing (2)
  • src/SDK/Language/Node.php
  • templates/node/src/services/template.ts.twig
🚧 Files skipped from review as they are similar to previous changes (2)
  • templates/node/src/services/template.ts.twig
  • src/SDK/Language/Node.php
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
  • GitHub Check: build (8.3, Swift56)
  • GitHub Check: build (8.3, Python39)
  • GitHub Check: build (8.3, Python313)
  • GitHub Check: build (8.3, KotlinJava8)
  • GitHub Check: build (8.3, KotlinJava17)
  • GitHub Check: build (8.3, DotNet80)
  • GitHub Check: build (8.3, Node18)
  • GitHub Check: build (8.3, Android14Java17)
  • GitHub Check: build (8.3, Go118)
  • GitHub Check: build (8.3, FlutterStable)
  • GitHub Check: build (8.3, KotlinJava11)
  • GitHub Check: build (8.3, DartStable)
  • GitHub Check: build (8.3, Node16)
  • GitHub Check: build (8.3, Android5Java17)
  • GitHub Check: flutter (client)
  • GitHub Check: swift (server)
  • GitHub Check: android (client)
  • GitHub Check: apple (client)

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@ItzNotABug ItzNotABug marked this pull request as ready for review January 10, 2026 09:29
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In @templates/node/src/inputFile.ts.twig:
- Around line 2-4: The module imports Node fs APIs at top-level (basename,
realpathSync, promises as fs) which breaks edge runtimes; change to
conditional/dynamic loading: remove top-level fs/realpathSync imports and
instead import or require them inside methods that need filesystem access (e.g.,
in InputFile.fromPath, InputFile.fromFileSystem) guarded by a runtime check/flag
similar to client.ts.twig, and provide safe fallbacks for edge environments
(e.g., have fromBuffer remain usable without fs, have realpathSync return the
input path or undefined in edge, and throw a clear error when file-system-only
methods are invoked in an edge runtime). Ensure references to
basename/realpathSync/fs are only used after the runtime check or dynamic import
so edge environments never attempt to load the Node fs module.
🧹 Nitpick comments (1)
templates/node/src/client.ts.twig (1)

217-261: LGTM - consider extracting shared chunking logic.

The InputFile chunked upload implementation correctly handles size detection, chunk slicing, progress tracking, and header propagation. The async/await usage with InputFile methods is appropriate.

The chunking logic between InputFile (lines 217-261) and File (lines 263-301) is largely duplicated. Consider extracting a shared helper, though this is optional given the different sync/async APIs.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 28217b1 and b6b9861.

📒 Files selected for processing (4)
  • src/SDK/Language/Node.php
  • templates/node/src/client.ts.twig
  • templates/node/src/inputFile.ts.twig
  • templates/node/src/services/template.ts.twig
🔇 Additional comments (6)
src/SDK/Language/Node.php (1)

76-77: LGTM!

The union type File | InputFile correctly allows both the standard File type and the new InputFile type for file parameters, enabling the chunked upload functionality while maintaining backward compatibility.

templates/node/src/services/template.ts.twig (1)

4-15: LGTM!

The conditional import logic correctly detects file parameters across all service methods and only imports InputFile when necessary, keeping the generated code clean and avoiding unused imports.

templates/node/src/client.ts.twig (1)

209-211: LGTM!

The file detection correctly handles both File and InputFile instances, enabling the chunked upload to work with either type.

templates/node/src/inputFile.ts.twig (3)

25-50: LGTM!

The fromBuffer factory correctly handles multiple input types with appropriate type detection:

  • BlobLike via duck-typing (arrayBuffer method check)
  • Buffer instances
  • Strings (encoded to Buffer)
  • ArrayBuffer (wrapped in Buffer)
  • TypedArrays (extracted with proper offset/length)

The fallback error for unsupported types provides clear feedback.


72-93: LGTM!

The slice method efficiently handles all source types:

  • Path-based: Uses file handles with proper cleanup via try/finally, and correctly handles partial reads
  • Buffer: Uses subarray for zero-copy slicing
  • Blob: Converts slice to Buffer via arrayBuffer

This enables true streaming for file-based sources without loading the entire file into memory.


95-109: LGTM!

The toFile and toBuffer methods correctly materialize the content for scenarios requiring full file access, such as small uploads that bypass chunking.

Comment thread templates/node/src/inputFile.ts.twig Outdated
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a 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

🧹 Nitpick comments (3)
tests/Node18Test.php (1)

30-33: Consider documenting the expected chunk count.

The test now expects exactly 4 large file upload responses, which aligns with the PR's chunking implementation. However, the hardcoded count of 4 chunks lacks context. If the test file size or CHUNK_SIZE constant changes, this expectation could silently break.

Consider adding a comment explaining why 4 chunks are expected (e.g., test file size ÷ chunk size), or better yet, verifying that the test file and chunk size configuration are documented to produce exactly 4 chunks.

💡 Optional: Improve comment clarity
-        ...Base::LARGE_FILE_RESPONSES, // Large file uploads
+        ...Base::LARGE_FILE_RESPONSES, // Large file upload (4 chunks expected)
tests/Node20Test.php (1)

30-33: Consider documenting the expected chunk count.

The test expects exactly 4 large file upload responses for chunked uploads. While this aligns with the PR's implementation, the hardcoded count lacks documentation about why 4 chunks are expected.

Consider adding context about the test file size and chunk size that result in exactly 4 chunks, or document this relationship to prevent silent test failures if configurations change.

💡 Optional: Improve comment clarity
-        ...Base::LARGE_FILE_RESPONSES, // Large file uploads
+        ...Base::LARGE_FILE_RESPONSES, // Large file upload (4 chunks expected)

Note: The change is consistent across all Node version tests (16, 18, 20), which is good for maintainability.

tests/Node16Test.php (1)

30-33: Consider documenting the expected chunk count and reducing duplication.

The test expects exactly 4 large file upload responses for chunked uploads, matching the pattern in Node18Test and Node20Test. While consistency across Node versions is excellent, the hardcoded count of 4 lacks documentation.

Additionally, since all three Node test classes (16, 18, 20) have identical chunk count expectations, consider:

  1. Documenting why 4 chunks are expected (test file size relationship to CHUNK_SIZE)
  2. Defining a constant in the Base class (e.g., LARGE_FILE_CHUNK_COUNT = 4) to make the expectation explicit and easier to maintain
♻️ Optional: Improve comment clarity and reduce duplication

Option 1: Improve the comment

-        ...Base::LARGE_FILE_RESPONSES, // Large file uploads
+        ...Base::LARGE_FILE_RESPONSES, // Large file upload (4 chunks expected)

Option 2: Use a constant in Base class

In tests/Base.php, add:

protected const LARGE_FILE_CHUNK_COUNT = 4; // Large file test expects 4 chunks based on file size and CHUNK_SIZE

Then in all Node test files:

...array_fill(0, Base::LARGE_FILE_CHUNK_COUNT, ...Base::LARGE_FILE_RESPONSES), // Large file upload chunks
📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b6b9861 and 0cd6e8b.

📒 Files selected for processing (3)
  • tests/Node16Test.php
  • tests/Node18Test.php
  • tests/Node20Test.php
🧰 Additional context used
🧬 Code graph analysis (2)
tests/Node18Test.php (1)
tests/Base.php (1)
  • Base (17-346)
tests/Node20Test.php (1)
tests/Base.php (1)
  • Base (17-346)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
  • GitHub Check: build (8.3, Python39)
  • GitHub Check: build (8.3, WebChromium)
  • GitHub Check: build (8.3, PHP80)
  • GitHub Check: build (8.3, Ruby30)
  • GitHub Check: build (8.3, Node20)
  • GitHub Check: build (8.3, PHP83)
  • GitHub Check: build (8.3, WebNode)
  • GitHub Check: build (8.3, Go118)
  • GitHub Check: build (8.3, KotlinJava17)
  • GitHub Check: build (8.3, Android5Java17)
  • GitHub Check: build (8.3, DartBeta)
  • GitHub Check: build (8.3, Android14Java17)
  • GitHub Check: build (8.3, DotNet90)
  • GitHub Check: build (8.3, FlutterBeta)
  • GitHub Check: apple (client)
  • GitHub Check: swift (server)
  • GitHub Check: android (client)

Comment thread templates/node/src/services/template.ts.twig Outdated
Copy link
Copy Markdown

Copilot AI commented Jan 12, 2026

@Meldiron I've opened a new pull request, #1297, to work on those changes. Once the pull request is ready, I'll request review from you.

Resolve Node upload template conflicts and keep InputFile edge-safe.
@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented Apr 20, 2026

Greptile Summary

This PR replaces the old eager readFileSync-based InputFile with a lazy, discriminated-union implementation (path / buffer / blob) and wires it into chunkedUpload so large path-backed files are streamed chunk-by-chunk rather than loaded into memory upfront. The File-path logic in chunkedUpload is untouched; the new InputFile branch mirrors it faithfully with correct content-range headers, x-appwrite-id carry-over, and onProgress callbacks. The edge-runtime guard (assertFileSystemAvailable) is well-placed at both creation time and at first I/O use. All findings are P2 style/maintenance suggestions.

Confidence Score: 5/5

Safe to merge — no P0/P1 issues; all findings are non-blocking P2 style suggestions.

The chunked upload logic is correct and mirrors the existing File path. The lazy fs loading and edge-runtime guard are implemented correctly. All remaining comments are future-proofing and style concerns that do not affect current correctness.

templates/node/src/inputFile.ts.twig — switch exhaustiveness and repeated getFs() import are worth addressing in a follow-up.

Important Files Changed

Filename Overview
templates/node/src/inputFile.ts.twig Replaces eager file-read with a lazy, discriminated-union source (path/buffer/blob). Core logic is correct; minor concern around missing exhaustive switch defaults and repeated getFs() import on every call.
templates/node/src/client.ts.twig Extends chunkedUpload to detect InputFile alongside File; small-file and chunked paths mirror the existing File logic correctly, with proper content-range headers and onProgress callbacks.
src/SDK/Language/Node.php Adds getTypeName override for File
templates/node/src/services/template.ts.twig Conditionally imports InputFile only when the service has file parameters via the new hasFileParam Twig filter — minimal, correct change.
tests/Node16Test.php Replaces duplicate UPLOAD_RESPONSES with four LARGE_FILE_RESPONSES entries to match new chunked upload test scenarios.
tests/Node18Test.php Same LARGE_FILE_RESPONSES update as Node16Test — consistent across all three node test versions.
tests/Node20Test.php Same LARGE_FILE_RESPONSES update as Node16/18Test — consistent across all three node test versions.

Reviews (2): Last reviewed commit: "Guard typed arrays before blob-like Inpu..." | Re-trigger Greptile

Comment thread templates/node/src/inputFile.ts.twig Outdated
@ChiragAgg5k ChiragAgg5k merged commit 525f063 into master Apr 20, 2026
57 checks passed
@ChiragAgg5k ChiragAgg5k deleted the fix-node-sync branch April 20, 2026 05:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants