-
Notifications
You must be signed in to change notification settings - Fork 24.4k
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
[TM] Add spec for BlobModule #24909
[TM] Add spec for BlobModule #24909
Conversation
Not sure that the id’s types are necessarily correct here…
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, but I think there's a mistake in the flow-typing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code analysis results:
flow
found some issues.
@@ -92,7 +92,7 @@ class BlobManager { | |||
} | |||
}, 0); | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cannot call NativeBlobModule.createFromParts
because property createFromParts
is missing in null or undefined [1].
@@ -131,38 +131,38 @@ class BlobManager { | |||
if (BlobRegistry.has(blobId)) { | |||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cannot call NativeBlobModule.release
because property release
is missing in null or undefined [1].
@@ -131,38 +131,38 @@ class BlobManager { | |||
if (BlobRegistry.has(blobId)) { | |||
return; | |||
} | |||
BlobModule.release(blobId); | |||
NativeBlobModule.release(blobId); | |||
} | |||
|
|||
/** | |||
* Inject the blob content handler in the networking module to support blob | |||
* requests and responses. | |||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cannot call NativeBlobModule.addNetworkingHandler
because property addNetworkingHandler
is missing in null or undefined [1].
} | ||
|
||
/** | ||
* Inject the blob content handler in the networking module to support blob | ||
* requests and responses. | ||
*/ | ||
static addNetworkingHandler(): void { | ||
BlobModule.addNetworkingHandler(); | ||
NativeBlobModule.addNetworkingHandler(); | ||
} | ||
|
||
/** | ||
* Indicate the websocket should return a blob for incoming binary | ||
* messages. | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cannot call NativeBlobModule.addWebSocketHandler
because property addWebSocketHandler
is missing in null or undefined [1].
} | ||
|
||
/** | ||
* Indicate the websocket should return a blob for incoming binary | ||
* messages. | ||
*/ | ||
static addWebSocketHandler(socketId: number): void { | ||
BlobModule.addWebSocketHandler(socketId); | ||
NativeBlobModule.addWebSocketHandler(socketId); | ||
} | ||
|
||
/** | ||
* Indicate the websocket should no longer return a blob for incoming | ||
* binary messages. | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cannot call NativeBlobModule.removeWebSocketHandler
because property removeWebSocketHandler
is missing in null or undefined [1].
} | ||
|
||
/** | ||
* Indicate the websocket should no longer return a blob for incoming | ||
* binary messages. | ||
*/ | ||
static removeWebSocketHandler(socketId: number): void { | ||
BlobModule.removeWebSocketHandler(socketId); | ||
NativeBlobModule.removeWebSocketHandler(socketId); | ||
} | ||
|
||
/** | ||
* Send a blob message to a websocket. | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cannot call NativeBlobModule.sendOverSocket
because property sendOverSocket
is missing in null or undefined [1].
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! Thanks for the fixes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RSNara has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
@RSNara flow is quite angry, i am attempting to fix |
This pull request was successfully merged by @ericlewis in 342c81d. When will my fix make it into a release? | Upcoming Releases |
Summary: Part of facebook#24875. Not sure that the id’s types are necessarily correct here… ## Changelog [General] [Added] - Add TM spec for BlobModule Pull Request resolved: facebook#24909 Reviewed By: fkgozali Differential Revision: D15433753 Pulled By: RSNara fbshipit-source-id: 68193d1a82fc7c66d6cc7ba4f22a0d3786987599
Summary
Part of #24875. Not sure that the id’s types are necessarily correct here…
Changelog
[General] [Added] - Add TM spec for BlobModule
Test Plan
Flow passes