Skip to content

Commit c4ae957

Browse files
hsubox76hsubox76
committed
Address PR comments
1 parent de75b56 commit c4ae957

File tree

12 files changed

+242
-168
lines changed

12 files changed

+242
-168
lines changed

‎packages/storage/compat/reference.ts

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@ import {
1919
Reference,
2020
getChild,
2121
uploadBytesResumable,
22-
uploadString,
2322
list,
2423
listAll,
2524
getDownloadURL,
@@ -29,12 +28,14 @@ import {
2928
} from '../src/reference';
3029
import * as types from '@firebase/storage-types';
3130
import { Metadata } from '../src/metadata';
32-
import { StringFormat } from '../src/implementation/string';
31+
import { dataFromString, StringFormat } from '../src/implementation/string';
3332
import { ListOptions } from '../src/list';
3433
import { UploadTaskCompat } from './task';
3534
import { ListResultCompat } from './list';
3635
import { StorageServiceCompat } from './service';
3736
import { invalidRootOperation } from '../src/implementation/error';
37+
import { UploadTask } from '../src/task';
38+
import { FbsBlob } from '../src/implementation/blob';
3839

3940
export class ReferenceCompat implements types.Reference {
4041
constructor(
@@ -114,8 +115,17 @@ export class ReferenceCompat implements types.Reference {
114115
metadata?: Metadata
115116
): types.UploadTask {
116117
this._throwIfRoot('putString');
118+
const data = dataFromString(format, value);
119+
const metadataClone = { ...metadata } as Metadata;
120+
if (metadataClone['contentType'] == null && data.contentType != null) {
121+
metadataClone['contentType'] = data.contentType!;
122+
}
117123
return new UploadTaskCompat(
118-
uploadString(this._delegate, value, format, metadata),
124+
new UploadTask(
125+
this._delegate,
126+
new FbsBlob(data.data, true),
127+
metadataClone
128+
),
119129
this
120130
);
121131
}

‎packages/storage/karma.conf.js

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -31,21 +31,20 @@ module.exports = function (config) {
3131
};
3232

3333
function getTestFiles(argv) {
34-
let unitTestFiles = [];
34+
let unitTestFiles = ['test/unit/*'];
3535
let integrationTestFiles = [];
3636
if (argv.exp) {
37-
unitTestFiles = ['test/unit/*'].filter(
37+
unitTestFiles = unitTestFiles.filter(
3838
filename => !filename.includes('.compat.')
3939
);
4040
integrationTestFiles = ['test/integration/*exp*'];
4141
} else if (argv.compat) {
42-
unitTestFiles = ['test/unit/*'].filter(
42+
unitTestFiles = unitTestFiles.filter(
4343
filename => !filename.includes('.exp.')
4444
);
4545
integrationTestFiles = ['test/integration/*compat*'];
4646
} else {
47-
console.log('Specify "exp" or "compat" option for karma command.');
48-
return;
47+
integrationTestFiles = ['test/integration/*'];
4948
}
5049
if (argv.unit) {
5150
return unitTestFiles;

‎packages/storage/package.json

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -17,15 +17,15 @@
1717
"build:exp": "rollup -c rollup.config.exp.js",
1818
"build:deps": "lerna run --scope @firebase/storage --include-dependencies build",
1919
"dev": "rollup -c -w",
20-
"test": "run-p test:browser:compat lint",
21-
"test:exp": "run-p test:browser:exp lint",
22-
"test:ci": "node ../../scripts/run_tests_in_ci.js -s test:browser:compat",
23-
"test:browser:compat:unit": "karma start --single-run --unit",
20+
"test": "run-p test:browser lint",
21+
"test:ci": "node ../../scripts/run_tests_in_ci.js -s test:browser",
22+
"test:browser:compat:unit": "karma start --single-run --compat --unit",
2423
"test:browser:exp:unit": "karma start --single-run --exp --unit",
25-
"test:browser:compat:integration": "karma start --single-run --integration",
24+
"test:browser:compat:integration": "karma start --single-run --compat --integration",
2625
"test:browser:exp:integration": "karma start --single-run --exp --integration",
2726
"test:browser:compat": "karma start --single-run --compat",
2827
"test:browser:exp": "karma start --single-run --exp",
28+
"test:browser": "karma start --single-run",
2929
"prepare": "yarn build",
3030
"prettier": "prettier --write 'src/**/*.ts' 'test/**/*.ts'",
3131
"api-report": "api-extractor run --local --verbose"

‎packages/storage/src/implementation/metadata.ts

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -20,11 +20,11 @@
2020
*/
2121
import { Metadata } from '../metadata';
2222

23-
import * as json from './json';
23+
import { jsonObjectOrNull } from './json';
2424
import { Location } from './location';
25-
import * as path from './path';
26-
import * as type from './type';
27-
import * as UrlUtils from './url';
25+
import { lastComponent } from './path';
26+
import { isString } from './type';
27+
import { makeUrl, makeQueryString } from './url';
2828
import { Reference } from '../reference';
2929
import { StorageService } from '../service';
3030

@@ -55,10 +55,10 @@ export { Mappings };
5555
let mappings_: Mappings | null = null;
5656

5757
export function xformPath(fullPath: string | undefined): string | undefined {
58-
if (!type.isString(fullPath) || fullPath.length < 2) {
58+
if (!isString(fullPath) || fullPath.length < 2) {
5959
return fullPath;
6060
} else {
61-
return path.lastComponent(fullPath);
61+
return lastComponent(fullPath);
6262
}
6363
}
6464

@@ -145,7 +145,7 @@ export function fromResourceString(
145145
resourceString: string,
146146
mappings: Mappings
147147
): Metadata | null {
148-
const obj = json.jsonObjectOrNull(resourceString);
148+
const obj = jsonObjectOrNull(resourceString);
149149
if (obj === null) {
150150
return null;
151151
}
@@ -157,11 +157,11 @@ export function downloadUrlFromResourceString(
157157
metadata: Metadata,
158158
resourceString: string
159159
): string | null {
160-
const obj = json.jsonObjectOrNull(resourceString);
160+
const obj = jsonObjectOrNull(resourceString);
161161
if (obj === null) {
162162
return null;
163163
}
164-
if (!type.isString(obj['downloadTokens'])) {
164+
if (!isString(obj['downloadTokens'])) {
165165
// This can happen if objects are uploaded through GCS and retrieved
166166
// through list, so we don't want to throw an Error.
167167
return null;
@@ -176,8 +176,8 @@ export function downloadUrlFromResourceString(
176176
const bucket: string = metadata['bucket'] as string;
177177
const path: string = metadata['fullPath'] as string;
178178
const urlPart = '/b/' + encode(bucket) + '/o/' + encode(path);
179-
const base = UrlUtils.makeUrl(urlPart);
180-
const queryString = UrlUtils.makeQueryString({
179+
const base = makeUrl(urlPart);
180+
const queryString = makeQueryString({
181181
alt: 'media',
182182
token
183183
});
@@ -187,7 +187,7 @@ export function downloadUrlFromResourceString(
187187
}
188188

189189
export function toResourceString(
190-
metadata: { [key: string]: unknown },
190+
metadata: Record<string, unknown>,
191191
mappings: Mappings
192192
): string {
193193
const resource: {

‎packages/storage/src/implementation/request.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -136,6 +136,8 @@ class NetworkRequest<T> implements Request<T> {
136136
xhr
137137
.send(self.url_, self.method_, self.body_, self.headers_)
138138
.then((xhr: XhrIo) => {
139+
console.log('response');
140+
console.log(xhr);
139141
if (self.progressCallback_ !== null) {
140142
xhr.removeUploadProgressListener(progressListener);
141143
}

‎packages/storage/src/implementation/requests.ts

Lines changed: 39 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -33,11 +33,16 @@ import {
3333
unknown
3434
} from './error';
3535
import { Location } from './location';
36-
import * as MetadataUtils from './metadata';
37-
import * as ListResultUtils from './list';
36+
import {
37+
Mappings,
38+
fromResourceString,
39+
downloadUrlFromResourceString,
40+
toResourceString
41+
} from './metadata';
42+
import { fromResponseString } from './list';
3843
import { RequestInfo, UrlParams } from './requestinfo';
39-
import * as type from './type';
40-
import * as UrlUtils from './url';
44+
import { isString } from './type';
45+
import { makeUrl } from './url';
4146
import { XhrIo } from './xhrio';
4247
import { StorageService } from '../service';
4348

@@ -52,10 +57,10 @@ export function handlerCheck(cndn: boolean): void {
5257

5358
export function metadataHandler(
5459
service: StorageService,
55-
mappings: MetadataUtils.Mappings
60+
mappings: Mappings
5661
): (p1: XhrIo, p2: string) => Metadata {
5762
function handler(xhr: XhrIo, text: string): Metadata {
58-
const metadata = MetadataUtils.fromResourceString(service, text, mappings);
63+
const metadata = fromResourceString(service, text, mappings);
5964
handlerCheck(metadata !== null);
6065
return metadata as Metadata;
6166
}
@@ -67,11 +72,7 @@ export function listHandler(
6772
bucket: string
6873
): (p1: XhrIo, p2: string) => ListResult {
6974
function handler(xhr: XhrIo, text: string): ListResult {
70-
const listResult = ListResultUtils.fromResponseString(
71-
service,
72-
bucket,
73-
text
74-
);
75+
const listResult = fromResponseString(service, bucket, text);
7576
handlerCheck(listResult !== null);
7677
return listResult as ListResult;
7778
}
@@ -80,15 +81,12 @@ export function listHandler(
8081

8182
export function downloadUrlHandler(
8283
service: StorageService,
83-
mappings: MetadataUtils.Mappings
84+
mappings: Mappings
8485
): (p1: XhrIo, p2: string) => string | null {
8586
function handler(xhr: XhrIo, text: string): string | null {
86-
const metadata = MetadataUtils.fromResourceString(service, text, mappings);
87+
const metadata = fromResourceString(service, text, mappings);
8788
handlerCheck(metadata !== null);
88-
return MetadataUtils.downloadUrlFromResourceString(
89-
metadata as Metadata,
90-
text
91-
);
89+
return downloadUrlFromResourceString(metadata as Metadata, text);
9290
}
9391
return handler;
9492
}
@@ -142,10 +140,10 @@ export function objectErrorHandler(
142140
export function getMetadata(
143141
service: StorageService,
144142
location: Location,
145-
mappings: MetadataUtils.Mappings
143+
mappings: Mappings
146144
): RequestInfo<Metadata> {
147145
const urlPart = location.fullServerUrl();
148-
const url = UrlUtils.makeUrl(urlPart);
146+
const url = makeUrl(urlPart);
149147
const method = 'GET';
150148
const timeout = service.maxOperationRetryTime;
151149
const requestInfo = new RequestInfo(
@@ -181,7 +179,7 @@ export function list(
181179
urlParams['maxResults'] = maxResults;
182180
}
183181
const urlPart = location.bucketOnlyServerUrl();
184-
const url = UrlUtils.makeUrl(urlPart);
182+
const url = makeUrl(urlPart);
185183
const method = 'GET';
186184
const timeout = service.maxOperationRetryTime;
187185
const requestInfo = new RequestInfo(
@@ -198,10 +196,10 @@ export function list(
198196
export function getDownloadUrl(
199197
service: StorageService,
200198
location: Location,
201-
mappings: MetadataUtils.Mappings
199+
mappings: Mappings
202200
): RequestInfo<string | null> {
203201
const urlPart = location.fullServerUrl();
204-
const url = UrlUtils.makeUrl(urlPart);
202+
const url = makeUrl(urlPart);
205203
const method = 'GET';
206204
const timeout = service.maxOperationRetryTime;
207205
const requestInfo = new RequestInfo(
@@ -217,13 +215,13 @@ export function getDownloadUrl(
217215
export function updateMetadata(
218216
service: StorageService,
219217
location: Location,
220-
metadata: { [key: string]: unknown },
221-
mappings: MetadataUtils.Mappings
218+
metadata: Record<string, unknown>,
219+
mappings: Mappings
222220
): RequestInfo<Metadata> {
223221
const urlPart = location.fullServerUrl();
224-
const url = UrlUtils.makeUrl(urlPart);
222+
const url = makeUrl(urlPart);
225223
const method = 'PATCH';
226-
const body = MetadataUtils.toResourceString(metadata, mappings);
224+
const body = toResourceString(metadata, mappings);
227225
const headers = { 'Content-Type': 'application/json; charset=utf-8' };
228226
const timeout = service.maxOperationRetryTime;
229227
const requestInfo = new RequestInfo(
@@ -243,7 +241,7 @@ export function deleteObject(
243241
location: Location
244242
): RequestInfo<void> {
245243
const urlPart = location.fullServerUrl();
246-
const url = UrlUtils.makeUrl(urlPart);
244+
const url = makeUrl(urlPart);
247245
const method = 'DELETE';
248246
const timeout = service.maxOperationRetryTime;
249247

@@ -285,18 +283,19 @@ export function metadataForUpload_(
285283
export function simpleUpload(
286284
service: StorageService,
287285
location: Location,
288-
mappings: MetadataUtils.Mappings,
286+
mappings: Mappings,
289287
blob: FbsBlob,
290288
metadata?: Metadata | null
291289
): RequestInfo<Metadata> {
292290
const urlPart = location.bucketOnlyServerUrl();
293-
const headers: { [prop: string]: string } = {
294-
'Content-Type': 'application/octet-stream'
295-
};
296291

297292
const metadata_ = metadataForUpload_(location, blob, metadata);
293+
const headers: { [prop: string]: string } = {
294+
// metadataForUpload_ always populates the contentType field.
295+
'Content-Type': metadata_['contentType']!
296+
};
298297
const urlParams: UrlParams = { name: metadata_['fullPath']! };
299-
const url = UrlUtils.makeUrl(urlPart);
298+
const url = makeUrl(urlPart);
300299
const method = 'POST';
301300
const timeout = service.maxUploadRetryTime;
302301
const requestInfo = new RequestInfo(
@@ -317,7 +316,7 @@ export function simpleUpload(
317316
export function multipartUpload(
318317
service: StorageService,
319318
location: Location,
320-
mappings: MetadataUtils.Mappings,
319+
mappings: Mappings,
321320
blob: FbsBlob,
322321
metadata?: Metadata | null
323322
): RequestInfo<Metadata> {
@@ -336,7 +335,7 @@ export function multipartUpload(
336335
const boundary = genBoundary();
337336
headers['Content-Type'] = 'multipart/related; boundary=' + boundary;
338337
const metadata_ = metadataForUpload_(location, blob, metadata);
339-
const metadataString = MetadataUtils.toResourceString(metadata_, mappings);
338+
const metadataString = toResourceString(metadata_, mappings);
340339
const preBlobPart =
341340
'--' +
342341
boundary +
@@ -355,7 +354,7 @@ export function multipartUpload(
355354
throw cannotSliceBlob();
356355
}
357356
const urlParams: UrlParams = { name: metadata_['fullPath']! };
358-
const url = UrlUtils.makeUrl(urlPart);
357+
const url = makeUrl(urlPart);
359358
const method = 'POST';
360359
const timeout = service.maxUploadRetryTime;
361360
const requestInfo = new RequestInfo(
@@ -408,14 +407,14 @@ export function checkResumeHeader_(xhr: XhrIo, allowed?: string[]): string {
408407
export function createResumableUpload(
409408
service: StorageService,
410409
location: Location,
411-
mappings: MetadataUtils.Mappings,
410+
mappings: Mappings,
412411
blob: FbsBlob,
413412
metadata?: Metadata | null
414413
): RequestInfo<string> {
415414
const urlPart = location.bucketOnlyServerUrl();
416415
const metadataForUpload = metadataForUpload_(location, blob, metadata);
417416
const urlParams: UrlParams = { name: metadataForUpload['fullPath']! };
418-
const url = UrlUtils.makeUrl(urlPart);
417+
const url = makeUrl(urlPart);
419418
const method = 'POST';
420419
const headers = {
421420
'X-Goog-Upload-Protocol': 'resumable',
@@ -424,7 +423,7 @@ export function createResumableUpload(
424423
'X-Goog-Upload-Header-Content-Type': metadataForUpload['contentType']!,
425424
'Content-Type': 'application/json; charset=utf-8'
426425
};
427-
const body = MetadataUtils.toResourceString(metadataForUpload, mappings);
426+
const body = toResourceString(metadataForUpload, mappings);
428427
const timeout = service.maxUploadRetryTime;
429428

430429
function handler(xhr: XhrIo): string {
@@ -435,7 +434,7 @@ export function createResumableUpload(
435434
} catch (e) {
436435
handlerCheck(false);
437436
}
438-
handlerCheck(type.isString(url));
437+
handlerCheck(isString(url));
439438
return url as string;
440439
}
441440
const requestInfo = new RequestInfo(url, method, handler, timeout);
@@ -504,7 +503,7 @@ export function continueResumableUpload(
504503
url: string,
505504
blob: FbsBlob,
506505
chunkSize: number,
507-
mappings: MetadataUtils.Mappings,
506+
mappings: Mappings,
508507
status?: ResumableUploadStatus | null,
509508
progressCallback?: ((p1: number, p2: number) => void) | null
510509
): RequestInfo<ResumableUploadStatus> {

0 commit comments

Comments
 (0)