[TM] Add spec for WebSocketModule

[jeanregisser]

Summary

Part of #24875

Changelog

[General] [Added] - Add TurboModule spec for WebSocketModule

Test Plan

Run yarn flow at the root level

[RSNara]

I've suggested two approaches for solving the close issue. Thoughts?

[RSNara]

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.

[jeanregisser]

Thanks @RSNara

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

3. 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.

[RSNara]

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.

[jeanregisser]

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.

[jeanregisser]

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.

[ericlewis]

If there is a misalignment of features that is fine, for now we just want a Spec, if you do think there is a misalignment of feature that can be fixed open a new PR :) thank you!! Sent via Superhuman iOS ( https://sprh.mn/?vip=ericlewis777@gmail.com )

…

On Fri, May 17 2019 at 4:14 AM, < ***@***.*** > wrote: ***@***.**** commented on this pull request. In Libraries/WebSocket/NativeWebSocketModule.js ( #24893 (comment) ) : > +// 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; Thanks @RSNara ( https://github.com/RSNara ) I was thinking about a 3rd approach, quite similar to approach 2: * 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: https://github.com/facebook/react-native/blob/7abfd23b90db08b426c3c91b0cb6d01d161a9b9e/Libraries/WebSocket/RCTSRWebSocket.m#L558 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. — You are receiving this because you commented. Reply to this email directly, view it on GitHub ( #24893?email_source=notifications&email_token=AAFEVR7OF6PLRQP5ADPWMCLPVZSNTA5CNFSM4HNLNQGKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOBY52O6Q#discussion_r285019804 ) , or mute the thread ( https://github.com/notifications/unsubscribe-auth/AAFEVR2LP253RQDCVHCE7Y3PVZSNTANCNFSM4HNLNQGA ).

[facebook-github-bot]

@fkgozali has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[react-native-bot]

This pull request was successfully merged by @jeanregisser in 5705ea1.

^{When will my fix make it into a release? | Upcoming Releases}