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

WebSocket polyfill #890

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
10 participants
@hharnisc
Contributor

hharnisc commented Apr 16, 2015

@hharnisc

This comment has been minimized.

Contributor

hharnisc commented Apr 17, 2015

I hear you there @tptee - though the source for socket rocket already lives the the react native library.

@enaros

This comment has been minimized.

enaros commented Apr 17, 2015

nice @hharnisc !

@naartjie

This comment has been minimized.

naartjie commented Apr 17, 2015

I believe the App Store will reject anything using SocketRocket until this issue is resolved: facebook/SocketRocket#226

@tptee any particular version of SocketRocket? I just had an app accepted which is using SocketRocket 0.2.0

@vjeux vjeux added the Needs Review label Apr 17, 2015

@lazywei

This comment has been minimized.

lazywei commented Apr 29, 2015

any news?

@nicklockwood

This comment has been minimized.

Contributor

nicklockwood commented Apr 29, 2015

Sorry, I've had this on the backburner for a little while. I'm just trying to figure out the best way to share the SocketRocket library between both this and the Chrome debugger, as they're both optional libraries.

@hharnisc

This comment has been minimized.

Contributor

hharnisc commented Apr 29, 2015

@nicklockwood understand where you're coming from. I tried a few different ways of using a shared library. Breaking it out into another project felt better than some of the other stuff I came up with. Would be great to use something like cocoapods to manage Obj-C dependencies.

@nicklockwood

This comment has been minimized.

Contributor

nicklockwood commented Apr 29, 2015

The options I've come up with so far are basically:

  1. Include SocketRocket in the core React lib, so both WebSocket and WebSocketExecutor can share it.
  2. Put the WebSocketExecutor and WebSocket in the same library.
  3. Have two copies of the SocketRocket library, and rename one so they don't clash if you include WebSocket and WebSocketExecutor in the same project (I'd probably also #ifdef out the WebSocketExecutor version in release mode to save a few KB)

Option 1 is probably the most straightforward, although it means that projects that don't use WebSocket have to carry the weight of SocketRocket in their binary anyway. Option 2 doesn't really solve anything. I guess I'm leaning towards option 3, but it will be a pain to maintain such a heavily modified fork of SR.

@nicklockwood

This comment has been minimized.

Contributor

nicklockwood commented Apr 29, 2015

The decision we've come to with CocoaPods is that we want to fully support people using it, but don't want to make it mandatory. Unfortunately that eliminates it as an option for solving mutual dependencies between our first party modules.

@hharnisc

This comment has been minimized.

Contributor

hharnisc commented Apr 29, 2015

I'm in favor of option 1, since it minimizes the size of the codebase -- I imagine the size of the binary will have more of an impact on initial download time and minimal impact on app load time.

Option 3 would be ok too but I think would be more confusing for developers (while working in the Obj-C or if they find a bug in a websocket related feature)

@lazywei

This comment has been minimized.

lazywei commented May 6, 2015

Same here .. I'd prefer option 1.

@frantic

This comment has been minimized.

Contributor

frantic commented May 6, 2015

IMHO it makes sense to ship built-in WebSocket implementation, we already have XHR.

@lazywei

This comment has been minimized.

lazywei commented May 6, 2015

To be honest, as an end-developer, I'm pretty OK with either way. All I want is just having the ability to develop program with WebSocket. To be more specific, I want to use Firebase.
This issue stops me from using Firebase for one month, and I really think it is important to have such ability in React Native. Simply put, I think a quick solution to this issue is the most important thing.
It's just my two cents though.

@jbuffin

This comment has been minimized.

jbuffin commented May 8, 2015

any idea when this might be merged in?

@lazywei

This comment has been minimized.

lazywei commented May 12, 2015

ping @nicklockwood
how is this going? is internal diff created means it is on the reviewing process?

Thanks

@nicklockwood

This comment has been minimized.

Contributor

nicklockwood commented May 14, 2015

@lazywel sorry for the delay. Internal diff means the github pull request has been translated into a pull request in our internal repository, which is the first step to being merged in.

This has now landed internally, so it should be included in the next update.

@vjeux vjeux reopened this May 14, 2015

@vjeux

This comment has been minimized.

Contributor

vjeux commented May 14, 2015

This actually breaks the tests

ld: library not found for -lRCTWebSocket

https://travis-ci.org/facebook/react-native/jobs/62613184

@nicklockwood

This comment has been minimized.

Contributor

nicklockwood commented May 14, 2015

@vjeux are you referring to the missing import in UIExplorer that@tadeuzagallo has now fixed? If so, can we close this again?

@vjeux vjeux closed this in babdc21 May 14, 2015

@vjeux

This comment has been minimized.

Contributor

vjeux commented May 14, 2015

@nicklockwood when I export the commits, all the related pull requests automatically get closed FYI :)

@lazywei

This comment has been minimized.

lazywei commented May 15, 2015

Wowwwwww!! Good to see it's been merged into the master!!
Thanks for all your great efforts!!!

@brysgo

This comment has been minimized.

brysgo commented May 15, 2015

Was the .xcodeproj left out of the final patch here? I've been trying to include this in my project and I'm fairly certain one is supposed to be there.

@nicklockwood

This comment has been minimized.

Contributor

nicklockwood commented May 15, 2015

@brysgo yeah, we messed up. We'll push up a fix shortly.

@brysgo

This comment has been minimized.

brysgo commented May 15, 2015

thanks 👍

@mikemintz mikemintz referenced this pull request Sep 12, 2015

Open

Support react-native #11

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment