Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix!: default to resumable with no threshold #1864

Merged
merged 4 commits into from
Apr 13, 2022
Merged

fix!: default to resumable with no threshold #1864

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9264707
fix: default to resumable with no threshold
shaffeeullah Apr 11, 2022
1c24310
fix documentaiton
shaffeeullah Apr 11, 2022
bf82d8b
linted file
shaffeeullah Apr 11, 2022
5ea139c
fixed documentation typo
shaffeeullah Apr 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 6 additions & 37 deletions src/bucket.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -362,8 +362,6 @@ export interface UploadOptions
destination?: string | File;
encryptionKey?: string | Buffer;
kmsKeyName?: string;
resumable?: boolean;
timeout?: number;
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this accidentally removed? It is still documented below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not accidentally removed and still documented below because it's still part of the interface. it's declared on CreateResumableUploadOptions. I removed this because it's duplicated

// eslint-disable-next-line @typescript-eslint/no-explicit-any
onUploadProgress?: (progressEvent: any) => void;
}
Expand Down Expand Up @@ -393,15 +391,6 @@ export enum BucketExceptionMessages {
SUPPLY_NOTIFICATION_ID = 'You must supply a notification ID.',
}

/**
* The size of a file (in bytes) must be greater than this number to
* automatically trigger a resumable upload.
*
* @const {number}
* @private
*/
const RESUMABLE_THRESHOLD = 5000000;

/**
* Get and set IAM policies for your bucket.
*
Expand Down Expand Up @@ -3699,8 +3688,8 @@ class Bucket extends ServiceObject {
* `options.predefinedAcl = 'private'`)
* @property {boolean} [public] Make the uploaded file public. (Alias for
* `options.predefinedAcl = 'publicRead'`)
* @property {boolean} [resumable] Force a resumable upload. (default:
* true for files larger than 5 MB).
* @property {boolean} [resumable=true] Resumable uploads are automatically
* enabled and must be shut off explicitly by setting to false.
* @property {number} [timeout=60000] Set the HTTP request timeout in
* milliseconds. This option is not available for resumable uploads.
* Default: `60000`
Expand Down Expand Up @@ -3729,9 +3718,7 @@ class Bucket extends ServiceObject {
* Upload a file to the bucket. This is a convenience method that wraps
* {@link File#createWriteStream}.
*
* You can specify whether or not an upload is resumable by setting
* `options.resumable`. *Resumable uploads are enabled by default if your
* input file is larger than 5 MB.*
* Resumable uploads are enabled by default
*
* For faster crc32c computation, you must manually install
* {@link https://www.npmjs.com/package/fast-crc32c| `fast-crc32c`}:
Expand Down Expand Up @@ -3784,8 +3771,8 @@ class Bucket extends ServiceObject {
* `options.predefinedAcl = 'private'`)
* @param {boolean} [options.public] Make the uploaded file public. (Alias for
* `options.predefinedAcl = 'publicRead'`)
* @param {boolean} [options.resumable] Force a resumable upload. (default:
* true for files larger than 5 MB).
* @property {boolean} [options.resumable=true] Resumable uploads are automatically
* enabled and must be shut off explicitly by setting to false.
* @param {number} [options.timeout=60000] Set the HTTP request timeout in
* milliseconds. This option is not available for resumable uploads.
* Default: `60000`
Expand Down Expand Up @@ -3826,7 +3813,6 @@ class Bucket extends ServiceObject {
* //-
* const options = {
* destination: 'new-image.png',
* resumable: true,
* validation: 'crc32c',
* metadata: {
* metadata: {
Expand Down Expand Up @@ -4029,24 +4015,7 @@ class Bucket extends ServiceObject {
});
}

if (options.resumable !== null && typeof options.resumable === 'boolean') {
upload(maxRetries);
} else {
// Determine if the upload should be resumable if it's over the threshold.
fs.stat(pathString, (err, fd) => {
if (err) {
callback!(err);
return;
}

if (fd.size <= RESUMABLE_THRESHOLD) {
// Only disable resumable uploads so createWriteStream still attempts them and falls back to simple upload.
options.resumable = false;
}

upload(maxRetries);
});
}
upload(maxRetries);
}

makeAllFilesPublicPrivate_(
Expand Down
38 changes: 2 additions & 36 deletions test/bucket.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2735,9 +2735,9 @@ describe('Bucket', () => {
};
});

it('should force a resumable upload', done => {
it('should respect setting a resumable upload to false', done => {
const fakeFile = new FakeFile(bucket, 'file-name');
const options = {destination: fakeFile, resumable: true};
const options = {destination: fakeFile, resumable: false};
fakeFile.createWriteStream = (options_: CreateWriteStreamOptions) => {
const ws = new stream.Writable();
ws.write = () => true;
Expand All @@ -2750,40 +2750,6 @@ describe('Bucket', () => {
bucket.upload(filepath, options, assert.ifError);
});

it('should not pass resumable option to createWriteStream when file size is greater than minimum resumable threshold', done => {
const fakeFile = new FakeFile(bucket, 'file-name');
const options = {destination: fakeFile};
fsStatOverride = (path: string, callback: Function) => {
// Set size greater than threshold
callback(null, {size: 5000001});
};
fakeFile.createWriteStream = (options_: CreateWriteStreamOptions) => {
const ws = new stream.Writable();
ws.write = () => true;
setImmediate(() => {
assert.strictEqual(typeof options_.resumable, 'undefined');
done();
});
return ws;
};
bucket.upload(filepath, options, assert.ifError);
});

it('should prevent resumable when file size is less than minimum resumable threshold', done => {
const fakeFile = new FakeFile(bucket, 'file-name');
const options = {destination: fakeFile};
fakeFile.createWriteStream = (options_: CreateWriteStreamOptions) => {
const ws = new stream.Writable();
ws.write = () => true;
setImmediate(() => {
assert.strictEqual(options_.resumable, false);
done();
});
return ws;
};
bucket.upload(filepath, options, assert.ifError);
});

it('should not retry a nonretryable error code', done => {
const fakeFile = new FakeFile(bucket, 'file-name');
const options = {destination: fakeFile, resumable: true};
Expand Down