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

Jump to bottom

Fix crashes in React Native "navigator is not defined" #55

Merged
merged 2 commits into from
Apr 3, 2016
Merged

Fix crashes in React Native "navigator is not defined" #55

merged 2 commits into from
Apr 3, 2016

Conversation

danawoodman
Copy link
Contributor

Ensures that navigator is defined before testing user agent so that Socket.io does not fail in React Native.

Tests are passing and I've also manually tested this and verified it works with React Native as expected.

Credit goes to @stevecass for originally reporting this over on Socket.io-client:
socketio/socket.io-client#945

@danawoodman
Fix crash in React Native
a10c487 
This change lets people use Socket.io in React Native

Related to an issue on socket.io-client repo: socketio/socket.io-client#945

All credit for this goes to @stevecass
@danawoodman
Ensure navigator is defined.
748144b
@danawoodman
Copy link
Contributor Author

@defunctzombie @rase- do yall need anything else from this PR to accept? Let me know 😄

@ekryski
Copy link

ekryski commented Mar 24, 2016

We're using a hack around this with Feathers currently, and it's now busted in React Native v0.22. Willing to lend a hand any way that I can to push this along as I think this is the proper fix. Looks like the build error was just a timeout or something.

This was referenced Mar 24, 2016
Closed
Closed
@danawoodman
Copy link
Contributor Author

Yeah seems tests actually pass, maybe a maintainer could have a look?

@ekryski
Copy link

ekryski commented Mar 31, 2016

@rauchg any chance you or anyone else at Automattic can give this a quick once over? Let us know if this feasible to get merged and what might need to change?

It's shitty because this requires quite a few releases to other modules like engine-io up the chain but I'd love to see if we can get this merged so that I don't have to fork this or find another workaround for React Native.

@rauchg rauchg merged commit 149f340 into socketio:master Apr 3, 2016
@rauchg
Copy link
Contributor

rauchg commented Apr 3, 2016

@ekryski this is very important. Thanks for pinging me.

@ekryski
Copy link

ekryski commented Apr 3, 2016

@rauchg no problemo! Thanks for the merge. 🎉 Mind pinging me when it goes out?

@ekryski ekryski mentioned this pull request Apr 3, 2016
Closed
@ekryski
Copy link

ekryski commented Apr 3, 2016

Actually come to think of it. This really should only be a patch release. So it should be a fairly quick update. I'll keep my 👀 on it.

@lopezjurip
Copy link

When this will get published?

@ekryski
Copy link

ekryski commented Apr 9, 2016

Well it's been merged. Just need @rauchg or someone else with access to do a new patch release. I know @rauchg has been pretty busy with his new project.

@rauchg
Copy link
Contributor

rauchg commented Apr 9, 2016

We're making a release on Monday

@danawoodman
Copy link
Contributor Author

Thanks for the merge @rauchg 😄 👍

@lopezjurip
Copy link

bump

@yberstad
Copy link

Any update on when the next release will be ready? 😄

@ekryski
Copy link

ekryski commented Apr 22, 2016

@rauchg I know you guys are slammed. Anything I can help with to get the release out the door? Quite a few people are waiting on this to fix socket.io + RN.

@polarathene
Copy link

@rauchg Could you please publish the update to NPM. Just updated React-Native and ran into this issue with socket.io.

@olivierlesnicki
Copy link

@rauchg is a release coming soon?

@ms88privat
Copy link

👍

@dozoisch
Copy link

Would love to see a release also!

@danawoodman
Copy link
Contributor Author

This appears to be released in 1.2.4 from what I can tell

@ekryski
Copy link

ekryski commented Jun 17, 2016

@danawoodman where are you seeing that? Looking through the commit logs it's not looking like it. Did you spelunk the code after installing it? I haven't revisited it with the latest releases of RN yet.

@danawoodman
Copy link
Contributor Author

@ekryski I saw it here: 1.2.4...master

@danawoodman
Copy link
Contributor Author

Wait, ignore me, that is changes from master. Guess it's still pending after all. @rauchg is there an ETA for including this?

@polarathene
Copy link

@danawoodman @rauchg has been fairly active on github but not responded to any of the pings here since April. Is there someone else that we can reach to get this fix released?

@rauchg if you're viewing this and able provide the ETA or any blockers please chime in and let us know, it would be greatly appreciated. If there is anything I can do, I'd be more than happy to.

@benjick
Copy link

benjick commented Jun 22, 2016

Bump

@polarathene
Copy link

@kapouer If you have some time and could help, that'd be great :)

@kapouer
Copy link

kapouer commented Jun 22, 2016

You know you can npm install https://github.com/socketio/engine.io-parser/archive/149f340.tar.gz right ? That's what i do when upstream is busy and i'm in a hurry...

@polarathene
Copy link

@kapouer I'm aware, though I prefer to only rely on that briefly. The issue has been steadily getting more activity with no response despite all the pings to @rauchg over the past two months .

I completely understand how busy things can get, however this is a minor fix to release if I'm not mistaken. I and others here would be happy to assist with any work required to get the release out.

@benjick
Copy link

benjick commented Jun 23, 2016

@kapouer Does that help me when I'm trying to use the socketio packages?

@danawoodman danawoodman mentioned this pull request Jun 30, 2016
Closed
@danawoodman
Copy link
Contributor Author

I've created a new issue to get some more visibility on this: #59

@Mentioum
Copy link

Is this coming?

@polarathene polarathene mentioned this pull request Aug 24, 2016
Closed
@danawoodman
Copy link
Contributor Author

BUMP BUMP BUMP BUMP BUMP

@razvanilin
Copy link

+1

@maximeaubaret
Copy link

+1 :)

@danawoodman
Copy link
Contributor Author

@darrachequesne forgive me if you're the wrong person to talk to but do you know when a new release will be pushed?

@Nibbler999 Nibbler999 mentioned this pull request Sep 16, 2016
Closed
@darrachequesne darrachequesne added this to the 1.3.0 milestone Oct 20, 2016
@darrachequesne darrachequesne mentioned this pull request Oct 21, 2016
Closed
@ekryski
Copy link

ekryski commented Nov 2, 2016

@darrachequesne thanks for publishing this mate! 🍻 Now we don't have to include any weird hacks in React Native apps. 🎉

@danawoodman
Copy link
Contributor Author

Holy $hit this finally got pushed! Thanks @darrachequesne, I guess you were the right one to nag! 😃

@skellock skellock mentioned this pull request Nov 27, 2016
Closed
@Mentioum
Copy link

@ekryski does that mean we can use socket.io nicely with the feathers client in React Native now? :) (just saw this).

@ekryski
Copy link

ekryski commented Dec 30, 2016

@Mentioum should be able to. We've written a few RN apps with sockets and it's working just fine. 😄

@danawoodman danawoodman deleted the patch-1 branch January 3, 2017 19:01
This was referenced Mar 1, 2017
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.3.0
Development

Successfully merging this pull request may close these issues.