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.

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

Implement Blob support for XMLHttpRequest #11573

Closed
wants to merge 76 commits into from
Closed
Show file tree
Hide file tree
Changes from 30 commits
Commits
Show all changes
76 commits
Select commit Hold shift + click to select a range
642d451
Add blob implementation with WebSocket integration
philikon Nov 25, 2016
be07fa9
Add a 'fromURI' method to blob
satya164 Nov 25, 2016
e8ea388
Fix blob not sliced when offset is 0
satya164 Nov 25, 2016
553f20e
Add blob support to XMLHttpRequest
satya164 Nov 25, 2016
aaba705
Make blob properties read-only following the spec
satya164 Dec 13, 2016
22ebafb
Add a basic implementation for FileReader on Android
satya164 Dec 16, 2016
781e9e6
Fix getting mime type
satya164 Dec 19, 2016
109a465
Add unit tests for Blob and File
satya164 Dec 20, 2016
2746628
Implement creating blob from string
satya164 Dec 21, 2016
a60605d
Add tests for blob module
satya164 Dec 21, 2016
d4b62b9
Cast blob parts to string
satya164 Dec 21, 2016
600217a
Fix counting bytes
satya164 Dec 21, 2016
b1614c1
Fix test for createFromParts
satya164 Dec 22, 2016
8ad6369
Don't allow accessing data after blob is closed
satya164 Dec 25, 2016
968a7b8
Keep track of blobs and release when blobs are released
satya164 Dec 25, 2016
60f8ebb
Fix typo in BlobManager
satya164 Jun 1, 2017
f56c66b
Use fetch to retrive blob from URI
satya164 Jun 6, 2017
7c0d8b2
Add support for downloading blobs on iOS
satya164 Jun 7, 2017
d62da30
Fix typo in base64 conversion
satya164 Jun 7, 2017
39feed9
Fix uploading blob via XHR
satya164 Jun 7, 2017
1a6060a
Basic implementation of file reader
satya164 Jun 8, 2017
8880eb9
Revert unwanted changes
satya164 Jun 8, 2017
9930649
Fix RNTester app
satya164 Jun 9, 2017
8b6fd3f
Fix BUCK
satya164 Jun 9, 2017
d0efa54
Respect specified encoding for readAsText
satya164 Jun 12, 2017
c2fa7ff
Initial merge
grabbou Aug 3, 2017
5c2dd2c
Attempt to merge #23102931
grabbou Aug 3, 2017
5fc2acb
Some fixes
satya164 Aug 3, 2017
e644c21
Make blob work on Android
satya164 Aug 11, 2017
9798801
Fix the file constructor and remove leftover file
satya164 Aug 27, 2017
8564ec8
Make blob module optional on Android
satya164 Sep 15, 2017
71b69ed
Merge branch 'master' into feat/blob
satya164 Sep 15, 2017
1479086
Fix compilation error on iOS
satya164 Sep 15, 2017
d9d3295
Remove dependency
RoninSTi Sep 29, 2017
ccf580e
Fixes, cleanup and @format
janicduplessis Oct 2, 2017
6159d5b
Xcodeproj fixes, add ios tests
janicduplessis Oct 3, 2017
0dbe167
Merge remote-tracking branch 'upstream/master' into feat/blob
janicduplessis Oct 4, 2017
cc9a425
Improve architecture and fix potential thread safety issues on iOS
janicduplessis Oct 4, 2017
ed2b250
Remove blob include
janicduplessis Oct 4, 2017
f928990
Revert websocket xcodeproj changes
janicduplessis Oct 4, 2017
35a37b6
Avoid statics for module data to make multiple instances play nicely,
janicduplessis Oct 5, 2017
adc5318
Fix whitespace p2
janicduplessis Oct 5, 2017
e70fb7e
Fix whitespace p3
janicduplessis Oct 5, 2017
37a0269
Improve buffer resize condition to match ios
janicduplessis Oct 5, 2017
1aa1f84
More cleanup
janicduplessis Oct 5, 2017
8e98f95
Fix buck files
janicduplessis Oct 5, 2017
45d014a
Document networking interfaces
janicduplessis Oct 5, 2017
21ff09b
Static NAME
janicduplessis Oct 5, 2017
cd6ede7
Fix flow
janicduplessis Oct 7, 2017
b536989
Fix buck build
janicduplessis Oct 7, 2017
75b4f1e
Fix android tests, cleanup createFromURI leftovers
janicduplessis Oct 7, 2017
d3a36ae
Merge branch 'master' into feat/blob
janicduplessis Oct 11, 2017
03165ca
Merge branch 'master' into feat/blob
janicduplessis Oct 17, 2017
3a28ad1
Merge branch 'master' into feat/blob
satya164 Nov 3, 2017
a977635
Merge branch 'master' into feat/blob
satya164 Nov 3, 2017
ebe159e
Merge branch 'master' into feat/blob
janicduplessis Nov 14, 2017
1de53b1
Merge branch 'master' into feat/blob
hramos Nov 16, 2017
ec88828
Merge branch 'master' into feat/blob
janicduplessis Jan 18, 2018
fb3496a
Fix invariant import, cleanup networking module imports
janicduplessis Jan 18, 2018
c243471
Address some review feedback
janicduplessis Jan 18, 2018
368b86e
Fix prettier
janicduplessis Jan 18, 2018
20cdd7e
Fix android build
janicduplessis Jan 18, 2018
84953bf
Replace ♥ with \u{2665}
hramos Jan 18, 2018
ddcbc64
Fix lint issue in BlobModuleTest.java
hramos Jan 18, 2018
4c7a928
Use correct unicode escapte
hramos Jan 18, 2018
da15540
Use string literal escapes for unicode
hramos Jan 18, 2018
28d1500
Update FileReaderModule.java
hramos Jan 18, 2018
00408ab
Update RCTBlobManagerTests.m
hramos Jan 18, 2018
236ed3f
Update RCTBlobManager.mm
hramos Jan 18, 2018
35e0c1c
Insert ⏎····· (prettier/prettier)
hramos Jan 18, 2018
e2481c4
Update Blob-test.js
hramos Jan 18, 2018
5d215ce
Update Blob-test.js
hramos Jan 18, 2018
5494fe9
Add back missing handler code that was lost in merge
janicduplessis Jan 18, 2018
ad568cf
Fix trailing whitespace on RCTBlobManager.mm
hramos Jan 18, 2018
2f15886
Import uuid from uuid/v4 to avoid conflict with internal fb haste module
janicduplessis Jan 23, 2018
d56239b
Fix test runner
janicduplessis Jan 23, 2018
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
89 changes: 39 additions & 50 deletions Libraries/Blob/Blob.js
Expand Up @@ -12,12 +12,7 @@

'use strict';

const invariant = require('fbjs/lib/invariant');
const uuid = require('uuid');

const { BlobModule } = require('NativeModules');

import type { BlobProps } from 'BlobTypes';
import type { BlobData, BlobOptions } from 'BlobTypes';

/**
* Opaque JS representation of some binary data in native.
Expand Down Expand Up @@ -57,61 +52,38 @@ import type { BlobProps } from 'BlobTypes';
* Reference: https://developer.mozilla.org/en-US/docs/Web/API/Blob
*/
class Blob {
/**
* Size of the data contained in the Blob object, in bytes.
*/
size: number;
/*
* String indicating the MIME type of the data contained in the Blob.
* If the type is unknown, this string is empty.
*/
type: string;

/*
* Unique id to identify the blob on native side (non-standard)
*/
blobId: string;
/*
* Offset to indicate part of blob, used when sliced (non-standard)
*/
offset: number;

/**
* Construct blob instance from blob data from native.
* Used internally by modules like XHR, WebSocket, etc.
*/
static create(props: BlobProps): Blob {
return Object.assign(Object.create(Blob.prototype), props);
}
_data: ?BlobData;

/**
* Constructor for JS consumers.
* Currently we only support creating Blobs from other Blobs.
* Reference: https://developer.mozilla.org/en-US/docs/Web/API/Blob/Blob
*/
constructor(parts: Array<Blob>, options: any) {
const blobId = uuid();
let size = 0;
parts.forEach((part) => {
invariant(part instanceof Blob, 'Can currently only create a Blob from other Blobs');
size += part.size;
});
BlobModule.createFromParts(parts, blobId);
return Blob.create({
blobId,
offset: 0,
size,
});
constructor(parts: Array<Blob | string> = [], options?: BlobOptions) {
const BlobManager = require('BlobManager');
Copy link
Contributor

Choose a reason for hiding this comment

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

To follow conventions of the repository (especially since initializing BlobManager does not have side effects), can you hoist these inlined require statements into the module scope?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can you hoist these inlined require statements into the module scope

It's not possible due to circular dependency between the Blob and BlobManager modules

this.data = BlobManager.createFromParts(parts, options).data;
}

/*
* This method is used to create a new Blob object containing
* the data in the specified range of bytes of the source Blob.
* Reference: https://developer.mozilla.org/en-US/docs/Web/API/Blob/slice
*/
set data(data: ?BlobData) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see any reason for making data writeable. Why not make Blob write to _data directly and replace the data getter with a getData method?

Also, these methods cause the docblock for slice to be misplaced. Please fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMO doesn't make sense to expose both _data and data to outside the module. It's more consistent use the same property name to read and write.

this._data = data;
}

get data(): BlobData {
if (this._data) {
return this._data;
}
throw new Error('Blob has been closed and is no longer available');
}

slice(start?: number, end?: number): Blob {
let offset = this.offset;
let size = this.size;
const BlobManager = require('BlobManager');
let { offset, size } = this.data;

if (typeof start === 'number') {
if (start > size) {
start = size;
Expand All @@ -126,8 +98,8 @@ class Blob {
size = end - start;
}
}
return Blob.create({
blobId: this.blobId,
return BlobManager.createFromOptions({
blobId: this.data.blobId,
offset,
size,
});
Expand All @@ -146,7 +118,24 @@ class Blob {
* `new Blob([blob, ...])` actually copies the data in memory.
*/
close() {
BlobModule.release(this.blobId);
const BlobManager = require('BlobManager');
BlobManager.release(this.data.blobId);
this.data = null;
}

/**
* Size of the data contained in the Blob object, in bytes.
*/
get size(): number {
Copy link
Contributor

Choose a reason for hiding this comment

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

If we remove the ability to mutate a Blob, can this just be a plain number property on each instance?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are read-only properties according to the Blob API. Replacing with plain property will make them writable.

return this.data.size;
}

/*
* String indicating the MIME type of the data contained in the Blob.
* If the type is unknown, this string is empty.
*/
get type(): string {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto.

return this.data.type || '';
}
}

Expand Down
88 changes: 88 additions & 0 deletions Libraries/Blob/BlobManager.js
@@ -0,0 +1,88 @@
/**
* Copyright (c) 2013-present, Facebook, Inc.
* All rights reserved.
*
* This source code is licensed under the BSD-style license found in the
* LICENSE file in the root directory of this source tree. An additional grant
* of patent rights can be found in the PATENTS file in the same directory.
*
* @providesModule BlobManager
* @flow
*/

'use strict';

const uuid = require('uuid');
const Blob = require('Blob');
const BlobRegistry = require('BlobRegistry');
const { BlobModule } = require('NativeModules');

import type { BlobData, BlobOptions } from 'BlobTypes';

/**
* Module to manage blobs
*/
class BlobManager {

/**
* Create blob from existing array of blobs.
*/
static createFromParts(parts: Array<Blob | string>, options?: BlobOptions): Blob {
const blobId = uuid.v4();
const items = parts.map(part => {
if (part instanceof ArrayBuffer || global.ArrayBufferView && part instanceof global.ArrayBufferView) {
throw new Error('Creating blobs from \'ArrayBuffer\' and \'ArrayBufferView\' are not supported');
}
if (part instanceof Blob) {
return {
data: part.data,
type: 'blob',
};
} else {
return {
data: String(part),
type: 'string',
};
}
});
const size = items.reduce((acc, curr) => {
if (curr.type === 'string') {
return acc + global.unescape(encodeURI(curr.data)).length;
} else {
return acc + curr.data.size;
}
}, 0);

BlobModule.createFromParts(items, blobId);

return BlobManager.createFromOptions({
blobId,
offset: 0,
size,
type: options ? options.type : '',
lastModified: options ? options.lastModified : Date.now(),
});
}

/**
* Create blob instance from blob data from native.
* Used internally by modules like XHR, WebSocket, etc.
*/
static createFromOptions(options: BlobData): Blob {
BlobRegistry.register(options.blobId);
return Object.assign(Object.create(Blob.prototype), { data: options });
}

/**
* Deallocate resources for a blob.
*/
static release(blobId: string) {
BlobRegistry.unregister(blobId);
if (BlobRegistry.has(blobId)) {
return;
}
BlobModule.release(blobId);
}
}

module.exports = BlobManager;
40 changes: 40 additions & 0 deletions Libraries/Blob/BlobRegistry.js
@@ -0,0 +1,40 @@
/**
* Copyright (c) 2013-present, Facebook, Inc.
* All rights reserved.
*
* This source code is licensed under the BSD-style license found in the
* LICENSE file in the root directory of this source tree. An additional grant
* of patent rights can be found in the PATENTS file in the same directory.
*
* @providesModule BlobRegistry
* @flow
*/

const registry: { [key: string]: number } = {};

const register = (id: string) => {
if (registry[id]) {
registry[id]++;
} else {
registry[id] = 1;
}
};

const unregister = (id: string) => {
if (registry[id]) {
registry[id]--;
if (registry[id] <= 0) {
delete registry[id];
}
}
};

const has = (id: string) => {
return registry[id] && registry[id] > 0;
};

module.exports = {
register,
unregister,
has,
};
8 changes: 5 additions & 3 deletions Libraries/Blob/BlobTypes.js
Expand Up @@ -12,14 +12,16 @@

'use strict';

export type BlobProps = {
export type BlobData = {
blobId: string,
offset: number,
size: number,
name?: string,
type?: string,
lastModified?: number,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add createdAt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Kureev Files only have a lastModifiedProperty https://developer.mozilla.org/en/docs/Web/API/File

};

export type FileProps = BlobProps & {
name: string,
export type BlobOptions = {
type: string,
lastModified: number,
};
46 changes: 46 additions & 0 deletions Libraries/Blob/File.js
@@ -0,0 +1,46 @@
/**
* Copyright (c) 2013-present, Facebook, Inc.
* All rights reserved.
*
* This source code is licensed under the BSD-style license found in the
* LICENSE file in the root directory of this source tree. An additional grant
* of patent rights can be found in the PATENTS file in the same directory.
*
* @providesModule File
* @flow
*/
'use strict';

const Blob = require('Blob');

import type { BlobOptions } from 'BlobTypes';

/**
* The File interface provides information about files.
*/
class File extends Blob {

/**
* Constructor for JS consumers.
*/
constructor(parts: Array<Blob | string> = [], name?: string, options?: BlobOptions) {
super(parts, options);
this.data.name = name;
Copy link
Contributor

Choose a reason for hiding this comment

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

Mutating this.data seems a bit like a code smell even though we know that the parent is currently the only one instantiating and managing it. I don't have any suggestions, though.

}

/**
* Name of the file.
*/
get name(): string {
return this.data.name || '';
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe it is impossible to have a file without name. Should we generate something like uuid for each blob w/o name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's valid, you can create a file object without name with new File([""], "");

Copy link
Contributor

Choose a reason for hiding this comment

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

Chrome throws if you don't pass a the parts and name args so I added an invariant. Behavior is slightly different since chrome accepts new File([], undefined) and stringifies it but I think it's better to not allow that. Still need the || '' to make flow happy though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add an invariant that this.data.name is a string instead of defaulting to '' (if it's just to make Flow happy). If it is anything else, then some constraint was violated between the constructor and invocation of this method.

}

/*
* Last modified time of the file.
*/
get lastModified(): number {
return this.data.lastModified || 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we set lastModified to createdAt by default? Not sure how it is handled in web, but on my Mac if I create a file, it sets created/modified/last opened by default to creation time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Kureev native side needs to pass this info when it's possible, but we can probably set it to current time from native if we cannot get the time.

Copy link
Contributor

Choose a reason for hiding this comment

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

Based on what I see as an outcome of new File([""], "");, lastModified is set to Date.now() by default

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto, this should just be an invariant that lastModified is a number (if it's just to make Flow happy).

}
}

module.exports = File;