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

[TM] Add spec for WebSocketModule #24893

Closed
wants to merge 5 commits into from
Closed

[TM] Add spec for WebSocketModule #24893

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6f58781
[TM] Add spec for WebSocketModule
jeanregisser May 16, 2019
6730a2d
[TM] Use NativeWebSocketModule in WebSocketInterceptor
jeanregisser May 16, 2019
5b75b7a
[TM] Update comment
jeanregisser May 16, 2019
05d31c1
[TM] Use 2 slightly different specs for now
jeanregisser May 19, 2019
a7dab07
Merge remote-tracking branch 'upstream/master' into spec-websocketmodule
jeanregisser May 20, 2019
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
38 changes: 38 additions & 0 deletions Libraries/WebSocket/NativeWebSocketModule.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
/**
* Copyright (c) Facebook, Inc. and its affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @flow
* @format
*/

'use strict';

import type {TurboModule} from 'RCTExport';
import * as TurboModuleRegistry from 'TurboModuleRegistry';

// Native close method definition on Android
declare function close(code: number, reason: string, socketID: number): void;
// Native close method definition on iOS
declare function close(socketID: number): void;

export interface Spec extends TurboModule {
+connect: (
url: string,
protocols: ?Array<string>,
options: ?{headers?: {origin?: string}},
socketID: number,
) => void;
+send: (message: string, socketID: number) => void;
+sendBinary: (base64String: string, socketID: number) => void;
+ping: (socketID: number) => void;
+close: typeof close;
Copy link
Contributor

@RSNara RSNara May 16, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

This is tricky.

Approaches

  1. Create the specs/{ios,android} folders. Then, have a spec for ios and a spec for android. The only thing different between the two would be the close method. Then, you could create a NativeWebSocketModule.js file that returns the correct implementation based on Platform.OS.
  2. Refactor the API of close on Android to be close(socketID: number, code: number, reason: string): void;. Then type the second and third argument as optional. It seems like the only place where we use WebSocketModule is in WebSocket.js, so this wouldn't be difficult. Also, while we're working on WebSocketModule, it might be worthwhile to actually do some cleanup. Also, with approach number one, we have a lot of duplicate code.

I'm leaning more towards option 2. Even though it's probably the more difficult option, and even though it is a breaking change, I think it's probably the better solution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @RSNara

I was thinking about a 3rd approach, quite similar to approach 2:

  1. Refactoring the iOS API and allowing closing the socket with the reason and code like on Android. I checked the underlying implementation and it seems like a viable approach:

    react-native/Libraries/WebSocket/RCTSRWebSocket.m

    Line 558 in 7abfd23

    - (void)closeWithCode:(NSInteger)code reason:(NSString *)reason;

This would actually make the iOS implementation more correct. Right now the JS interface for closing the WebSocket supports the reason and code but ignores it on iOS.

If we go this way, should the refactor be done in a different PR? I assume yes, but just checking.

Let me know what you think.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's also a good idea! But unfortunately, I just realized that our fb-internal infra doesn't allow refactoring NativeModule methods. So for the time being, I think we should just create two different spec files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not too sure what you mean about the fb-internal infra refactoring limitation, but I've created 2 slightly different specs for now in 05d31c1

Note that I had to suppress the flow error when calling the close method. Let me know if there's a better way to avoid this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright since my small refactor for the iOS close method got merged, I'm gonna update the spec! There's no more a difference between the native iOS and Android implementations.


// Events
+addListener: (eventName: string) => void;
+removeListeners: (count: number) => void;
}

export default TurboModuleRegistry.getEnforcing<Spec>('WebSocketModule');
17 changes: 8 additions & 9 deletions Libraries/WebSocket/WebSocket.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,15 +14,14 @@ const Blob = require('../Blob/Blob');
const EventTarget = require('event-target-shim');
const NativeEventEmitter = require('../EventEmitter/NativeEventEmitter');
const BlobManager = require('../Blob/BlobManager');
const NativeModules = require('../BatchedBridge/NativeModules');
const Platform = require('../Utilities/Platform');
const WebSocketEvent = require('./WebSocketEvent');

const base64 = require('base64-js');
const binaryToBase64 = require('../Utilities/binaryToBase64');
const invariant = require('invariant');

const {WebSocketModule} = NativeModules;
import NativeWebSocketModule from './NativeWebSocketModule';

import type EventSubscription from '../vendor/emitter/EventSubscription';

Expand Down Expand Up @@ -128,10 +127,10 @@ class WebSocket extends EventTarget(...WEBSOCKET_EVENTS) {
protocols = null;
}

this._eventEmitter = new NativeEventEmitter(WebSocketModule);
this._eventEmitter = new NativeEventEmitter(NativeWebSocketModule);
this._socketId = nextWebSocketId++;
this._registerEvents();
WebSocketModule.connect(url, protocols, {headers}, this._socketId);
NativeWebSocketModule.connect(url, protocols, {headers}, this._socketId);
}

get binaryType(): ?BinaryType {
Expand Down Expand Up @@ -180,12 +179,12 @@ class WebSocket extends EventTarget(...WEBSOCKET_EVENTS) {
}

if (typeof data === 'string') {
WebSocketModule.send(data, this._socketId);
NativeWebSocketModule.send(data, this._socketId);
return;
}

if (data instanceof ArrayBuffer || ArrayBuffer.isView(data)) {
WebSocketModule.sendBinary(binaryToBase64(data), this._socketId);
NativeWebSocketModule.sendBinary(binaryToBase64(data), this._socketId);
return;
}

Expand All @@ -197,17 +196,17 @@ class WebSocket extends EventTarget(...WEBSOCKET_EVENTS) {
throw new Error('INVALID_STATE_ERR');
}

WebSocketModule.ping(this._socketId);
NativeWebSocketModule.ping(this._socketId);
}

_close(code?: number, reason?: string): void {
if (Platform.OS === 'android') {
// See https://developer.mozilla.org/en-US/docs/Web/API/CloseEvent
const statusCode = typeof code === 'number' ? code : CLOSE_NORMAL;
const closeReason = typeof reason === 'string' ? reason : '';
WebSocketModule.close(statusCode, closeReason, this._socketId);
NativeWebSocketModule.close(statusCode, closeReason, this._socketId);
} else {
WebSocketModule.close(this._socketId);
NativeWebSocketModule.close(this._socketId);
}

if (BlobManager.isAvailable && this._binaryType === 'blob') {
Expand Down
35 changes: 20 additions & 15 deletions Libraries/WebSocket/WebSocketInterceptor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,16 +9,16 @@

'use strict';

const RCTWebSocketModule = require('../BatchedBridge/NativeModules')
.WebSocketModule;
const NativeEventEmitter = require('../EventEmitter/NativeEventEmitter');

import NativeWebSocketModule from './NativeWebSocketModule';

const base64 = require('base64-js');

const originalRCTWebSocketConnect = RCTWebSocketModule.connect;
const originalRCTWebSocketSend = RCTWebSocketModule.send;
const originalRCTWebSocketSendBinary = RCTWebSocketModule.sendBinary;
const originalRCTWebSocketClose = RCTWebSocketModule.close;
const originalRCTWebSocketConnect = NativeWebSocketModule.connect;
const originalRCTWebSocketSend = NativeWebSocketModule.send;
const originalRCTWebSocketSendBinary = NativeWebSocketModule.sendBinary;
const originalRCTWebSocketClose = NativeWebSocketModule.close;

let eventEmitter: NativeEventEmitter;
let subscriptions: Array<EventSubscription>;
Expand Down Expand Up @@ -135,13 +135,18 @@ const WebSocketInterceptor = {
if (isInterceptorEnabled) {
return;
}
eventEmitter = new NativeEventEmitter(RCTWebSocketModule);
eventEmitter = new NativeEventEmitter(NativeWebSocketModule);
WebSocketInterceptor._registerEvents();

// Override `connect` method for all RCTWebSocketModule requests
// to intercept the request url, protocols, options and socketId,
// then pass them through the `connectCallback`.
RCTWebSocketModule.connect = function(url, protocols, options, socketId) {
NativeWebSocketModule.connect = function(
url,
protocols,
options,
socketId,
) {
if (connectCallback) {
connectCallback(url, protocols, options, socketId);
}
Expand All @@ -150,7 +155,7 @@ const WebSocketInterceptor = {

// Override `send` method for all RCTWebSocketModule requests to intercept
// the data sent, then pass them through the `sendCallback`.
RCTWebSocketModule.send = function(data, socketId) {
NativeWebSocketModule.send = function(data, socketId) {
if (sendCallback) {
sendCallback(data, socketId);
}
Expand All @@ -159,7 +164,7 @@ const WebSocketInterceptor = {

// Override `sendBinary` method for all RCTWebSocketModule requests to
// intercept the data sent, then pass them through the `sendCallback`.
RCTWebSocketModule.sendBinary = function(data, socketId) {
NativeWebSocketModule.sendBinary = function(data, socketId) {
if (sendCallback) {
sendCallback(WebSocketInterceptor._arrayBufferToString(data), socketId);
}
Expand All @@ -168,7 +173,7 @@ const WebSocketInterceptor = {

// Override `close` method for all RCTWebSocketModule requests to intercept
// the close information, then pass them through the `closeCallback`.
RCTWebSocketModule.close = function() {
NativeWebSocketModule.close = function() {
if (closeCallback) {
if (arguments.length === 3) {
closeCallback(arguments[0], arguments[1], arguments[2]);
Expand Down Expand Up @@ -203,10 +208,10 @@ const WebSocketInterceptor = {
return;
}
isInterceptorEnabled = false;
RCTWebSocketModule.send = originalRCTWebSocketSend;
RCTWebSocketModule.sendBinary = originalRCTWebSocketSendBinary;
RCTWebSocketModule.close = originalRCTWebSocketClose;
RCTWebSocketModule.connect = originalRCTWebSocketConnect;
NativeWebSocketModule.send = originalRCTWebSocketSend;
NativeWebSocketModule.sendBinary = originalRCTWebSocketSendBinary;
NativeWebSocketModule.close = originalRCTWebSocketClose;
NativeWebSocketModule.connect = originalRCTWebSocketConnect;

connectCallback = null;
closeCallback = null;
Expand Down