Replace fs.ReadStream-centric handling with more generic PathReference (#767)

Author: Acconut

demos/nodejs/index.js

@@ -1,8 +1,7 @@
-import { createReadStream } from 'node:fs'
 import { Upload } from 'tus-js-client'
 
 const path = `${import.meta.dirname}/../../README.md`
-const file = createReadStream(path)
+const file = { path }
 
 const options = {
   endpoint: 'https://tusd.tusdemo.net/files/',

lib/node/NodeFileReader.ts

@@ -1,18 +1,27 @@
-import { ReadStream } from 'node:fs'
+import { createReadStream } from 'node:fs'
 import isStream from 'is-stream'
 
 import {
   openFile as openBaseFile,
   supportedTypes as supportedBaseTypes,
 } from '../commonFileReader.js'
-import type { FileReader, UploadInput } from '../options.js'
-import { getFileSource } from './sources/NodeFileSource.js'
+import type { FileReader, PathReference, UploadInput } from '../options.js'
 import { NodeStreamFileSource } from './sources/NodeStreamFileSource.js'
+import { getFileSourceFromPath } from './sources/PathFileSource.js'
+
+function isPathReference(input: UploadInput): input is PathReference {
+  return (
+    typeof input === 'object' &&
+    input !== null &&
+    'path' in input &&
+    (typeof input.path === 'string' || Buffer.isBuffer(input.path))
+  )
+}
 
 export class NodeFileReader implements FileReader {
   openFile(input: UploadInput, chunkSize: number) {
-    if (input instanceof ReadStream && input.path != null) {
-      return getFileSource(input)
+    if (isPathReference(input)) {
+      return getFileSourceFromPath(input)
     }
 
     if (isStream.readable(input)) {
@@ -33,3 +42,19 @@ export class NodeFileReader implements FileReader {
     )
   }
 }
+
+/**
+ * This (unused) function is a simple test to ensure that fs.ReadStreams
+ * satisfy the PathReference interface. In the past, tus-js-client explicitly
+ * accepted fs.ReadStreams and included it in its type definitions.
+ *
+ * Since tus-js-client v5, we have moved away from only accepting fs.ReadStream
+ * in favor of a more generic PathReference. This function ensures that the definition
+ * of PathReference includes fs.ReadStream. If this wasn't the case, the TypeScript
+ * compiler would complain during the build step, making this a poor-man's type test.
+ */
+// biome-ignore lint/correctness/noUnusedVariables: see above
+function testFsReadStreamAsPathReference() {
+  const pathReference: PathReference = createReadStream('test.txt')
+  new NodeFileReader().openFile(pathReference, 1024)
+}

lib/node/index.ts

@@ -3,7 +3,7 @@ import type { Readable } from 'node:stream'
 import { DetailedError } from '../DetailedError.js'
 import { NoopUrlStorage } from '../NoopUrlStorage.js'
 import { enableDebugLog } from '../logger.js'
-import type { UploadInput, UploadOptions } from '../options.js'
+import type { PathReference, UploadInput, UploadOptions } from '../options.js'
 import { BaseUpload, defaultOptions as baseDefaultOptions, terminate } from '../upload.js'
 
 import { canStoreURLs } from './FileUrlStorage.js'
@@ -25,7 +25,8 @@ export type FileTypes =
   | ArrayBufferView
   | Blob
   | Readable
-  | ReadStream
+  | PathReference
+
 export type FileSliceTypes = ArrayBuffer | SharedArrayBuffer | ArrayBufferView | Blob | ReadStream
 
 class Upload extends BaseUpload {

lib/node/sources/NodeFileSource.ts renamed to lib/node/sources/PathFileSource.ts

@@ -1,50 +1,47 @@
 import { type ReadStream, createReadStream, promises as fsPromises } from 'node:fs'
-import type { FileSource } from '../../options.js'
+import type { FileSource, PathReference } from '../../options.js'
 
-export async function getFileSource(stream: ReadStream): Promise<NodeFileSource> {
-  const path = stream.path.toString()
+export async function getFileSourceFromPath(file: PathReference): Promise<PathFileSource> {
+  const path = file.path.toString()
   const { size } = await fsPromises.stat(path)
 
-  // The fs.ReadStream might be configured to not read from the beginning
+  // The path reference might be configured to not read from the beginning
   // to the end, but instead from a slice in between. In this case, we consider
   // that range to indicate the actual uploadable size.
-  // This happens, for example, if a fs.ReadStream is used with the `parallelUploads`
-  // option. There, the ReadStream is sliced into multiple ReadStreams to fit the number
-  // of number of `parallelUploads`. Each ReadStream has `start` and `end` set.
+  // This happens, for example, if a path reference is used with the `parallelUploads`
+  // option. There, the path reference is sliced into multiple fs.ReadStreams to fit the
+  // number of `parallelUploads`. Each ReadStream has `start` and `end` set.
   // Note: `stream.end` is Infinity by default, so we need the check `isFinite`.
   // Note: `stream.end` is treated inclusively, so we need to add 1 here.
   // See the comment in slice() for more context.
-  // @ts-expect-error The types don't know start yet.
-  const start = stream.start ?? 0
-  // @ts-expect-error The types don't know end yet.
-  const end = Number.isFinite(stream.end) ? stream.end + 1 : size
+  const start = file.start ?? 0
+  const end = file.end != null && Number.isFinite(file.end) ? file.end + 1 : size
   const actualSize = end - start
 
-  return new NodeFileSource(stream, path, actualSize)
+  return new PathFileSource(file, path, actualSize)
 }
 
-export class NodeFileSource implements FileSource {
+export class PathFileSource implements FileSource {
   size: number
 
-  private _stream: ReadStream
+  private _file: PathReference
 
   private _path: string
 
-  constructor(stream: ReadStream, path: string, size: number) {
-    this._stream = stream
+  constructor(file: PathReference, path: string, size: number) {
+    this._file = file
     this._path = path
     this.size = size
   }
 
   slice(start: number, end: number) {
-    // The fs.ReadStream might be configured to not read from the beginning,
+    // The path reference might be configured to not read from the beginning,
     // but instead start at a different offset. The start value from the caller
     // does not include the offset, so we need to add this offset to our range later.
-    // This happens, for example, if a fs.ReadStream is used with the `parallelUploads`
-    // option. First, the ReadStream is sliced into multiple ReadStreams to fit the number
-    // of number of `parallelUploads`. Each ReadStream has `start` set.
-    // @ts-expect-error The types don't know start yet.
-    const offset = this._stream.start ?? 0
+    // This happens, for example, if a path reference is used with the `parallelUploads`
+    // option. First, the path reference is sliced into multiple fs.ReadStreams to fit the
+    // number of `parallelUploads`. Each ReadStream has `start` set.
+    const offset = this._file.start ?? 0
 
     const stream: ReadStream & { size?: number } = createReadStream(this._path, {
       start: offset + start,
@@ -62,6 +59,7 @@ export class NodeFileSource implements FileSource {
   }
 
   close() {
-    this._stream.destroy()
+    // TODO: Ensure that the read streams are closed
+    // TODO: Previously, the passed fs.ReadStream was closed here. Should we keep this behavior? If not, this is a breaking change.
   }
 }

lib/options.ts

@@ -1,23 +1,40 @@
-import type { ReadStream } from 'node:fs'
 import type { Readable } from 'node:stream'
 import type { DetailedError } from './DetailedError.js'
 
 export const PROTOCOL_TUS_V1 = 'tus-v1'
 export const PROTOCOL_IETF_DRAFT_03 = 'ietf-draft-03'
 export const PROTOCOL_IETF_DRAFT_05 = 'ietf-draft-05'
 
-// ReactNativeFile describes the structure that is returned from the
-// Expo image picker (see https://docs.expo.dev/versions/latest/sdk/imagepicker/)
-// TODO: Should these properties be fileName and fileSize instead?
-// TODO: What about other file pickers without Expo?
-// TODO: Should this be renamed to Expo?
+/**
+ * ReactNativeFile describes the structure that is returned from the
+ * Expo image picker (see https://docs.expo.dev/versions/latest/sdk/imagepicker/)
+ * TODO: Should these properties be fileName and fileSize instead?
+ * TODO: What about other file pickers without Expo?
+ * TODO: Should this be renamed to Expo?
+ * TODO: Only size is relevant for us. Not the rest.
+ */
 export interface ReactNativeFile {
   uri: string
   name?: string
   size?: string
   exif?: Record<string, unknown>
 }
 
+/**
+ * PathReference is a reference to a file on disk. Currently, it's only supported
+ * in Node.js. It can be supplied as a normal object or as an instance of `fs.ReadStream`,
+ * which also statisfies this interface.
+ *
+ * Optionally, a start and/or end position can be defined to define a range of bytes from
+ * the file that should be uploaded instead of the entire file. Both start and end are
+ * inclusive and start counting at 0, similar to the options accepted by `fs.createReadStream`.
+ */
+export interface PathReference {
+  path: string | Buffer
+  start?: number
+  end?: number
+}
+
 export type UploadInput =
   // Blob, File, ReadableStreamDefaultReader are available in browsers and Node.js
   | Blob // includes File
@@ -29,8 +46,7 @@ export type UploadInput =
   | Pick<ReadableStreamDefaultReader, 'read'>
   // Buffer, stream.Readable, fs.ReadStream are available in Node.js
   | Readable // TODO: Replace this with our own interface based on https://github.com/DefinitelyTyped/DefinitelyTyped/blob/3634b01d50c10ce1afaae63e41d39e7da309d8e3/types/node/globals.d.ts#L399
-  | ReadStream // TODO: Replace this with { path: string, start: number?, end: number? }
-  // ReactNativeFile is intended for React Native apps
+  | PathReference
   | ReactNativeFile
 
 export interface UploadOptions {

test/spec/test-node-specific.js

@@ -4,6 +4,7 @@ import fs from 'node:fs'
 import http from 'node:http'
 import https from 'node:https'
 import stream from 'node:stream'
+import { text } from 'node:stream/consumers'
 import intoStream from 'into-stream'
 import temp from 'temp'
 import { Upload, canStoreURLs } from 'tus-js-client'
@@ -32,7 +33,7 @@ describe('tus', () => {
       await expectHelloWorldUpload(buffer, options)
     })
 
-    describe('uploading from a stream', () => {
+    describe('uploading from a Node.js stream', () => {
       it('should reject streams without specifying the size', async () => {
         const input = new stream.PassThrough()
         const options = {
@@ -141,28 +142,30 @@ describe('tus', () => {
       })
     })
 
-    describe('uploading from a fs.ReadStream', () => {
+    describe('uploading from a file on disk', () => {
       it('should accept fs.ReadStream', async () => {
         // Create a temporary file
         const path = temp.path()
-        fs.writeFileSync(path, 'hello world')
-        const file = fs.createReadStream(path)
+        fs.writeFileSync(path, 'XXXhello worldXXX')
+        const file1 = fs.createReadStream(path, { start: 3, end: 13 })
+        expect(await text(file1)).toBe('hello world') // ensure that tus-js-client uploads the same data as fs.createReadStream
+        const file2 = fs.createReadStream(path, { start: 3, end: 13 })
 
         const options = {
           httpStack: new TestHttpStack(),
           endpoint: '/uploads',
           chunkSize: 7,
         }
 
-        await expectHelloWorldUpload(file, options)
+        await expectHelloWorldUpload(file2, options)
       })
 
       it('should support parallelUploads', async () => {
         // TODO: The ordering of requests is no longer deterministic, so we need to update this test
         // Create a temporary file
         const path = temp.path()
         fs.writeFileSync(path, 'hello world')
-        const file = fs.createReadStream(path)
+        const file = { path }
 
         const testStack = new TestHttpStack()
 
@@ -263,7 +266,7 @@ describe('tus', () => {
         // Create a temporary file
         const path = temp.path()
         fs.writeFileSync(path, 'hello world')
-        const file = fs.createReadStream(path)
+        const file = { path }
 
         const testStack = new TestHttpStack()
         const options = {