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.

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

FR: Firestore Web SDK React-Native/Expo support for Android devices #283

Closed
samsafay opened this issue Oct 31, 2017 · 46 comments

Comments

Projects
None yet
@samsafay
Copy link

commented Oct 31, 2017

Describe your environment

  • Operating System version: Android 4.4.2
  • Firebase SDK version: "^4.6.0"
  • Firebase Product: Firestore Web SDK
  • Expo: "^22.0.0"

Describe the problem

I am using Expo and trying to do basic CRUD with Firestore on an Android device and not getting any results. The same code works perfectly on an iOS device, but it doesn't work with Android. Can you take a look into that?

Steps to reproduce:

Basically create a react-native project, and Try any CRUD with Firestore. In my case fetching data from restaurants collection. Note, I am implementing Redux so you have to install 'react-redux' with your Expo project. As I said this code works perfectly with iOS but not with ANDROID. Let me know if you need more information.
Just tried this on react-native init project and got this error on my emulator. I just realized that this issue is related to #97.
1

Relevant Code:

import firebase from 'firebase';
import 'firebase/firestore';
import { Alert } from 'react-native';
import {
    RESTAURANTS_FETCH_SUCCESS,
} from './types';

export const restaurantsFetch = () => async dispatch => {
    try {
        const response = await firebase.firestore().collection('restaurants').get();
        response.forEach(doc => {
            dispatch({ type: RESTAURANTS_FETCH_SUCCESS, payload: { ...doc.data(), id: doc.id } });
        });
    } catch (err) {
        Alert.alert(err.message);
    }
};
@google-oss-bot

This comment has been minimized.

Copy link
Collaborator

commented Oct 31, 2017

Hmmm this issue does not seem to follow the issue template. Make sure you provide all the required information.

@fweffort

This comment has been minimized.

Copy link

commented Nov 1, 2017

hmmm this answer is not very welcoming to people trying to use your new product(firestore) and helping you identify errors.
It does not follow the issue template, but it's perfectly understandable.

Motivation for or Use Case - It is an error for everyone trying to use expo+firestore on ANDROID
Related Issues - Nope!
Environment Configuration -
It is a problem with android only,on your COMPETITOR's, ios, works fine, shouldn't android work flawlessly with google firestore?

Reproduce the Error -
create an app with create-react-native-app, following the guides from the documentation,
try and query any collection using ios and console.log the output. IT WORKS
same result is expected using android, but no error is thrown, and neither any result shown.

Why is it important?
Several new developers are using CRNA, following the documentation leads to Expo, and people enjoy it, people are going to start developing for ios only, or not using your brand new product at all.

Thanks for the welcoming answers on people trying to help you make better products!
Cheers!

@samsafay samsafay changed the title Firestore React-Native/Expo support for Android devices FR: Firestore React-Native/Expo support for Android devices Nov 2, 2017

@samsafay samsafay changed the title FR: Firestore React-Native/Expo support for Android devices FR: Firestore Web SDK React-Native/Expo support for Android devices Nov 2, 2017

@czubocha

This comment has been minimized.

Copy link

commented Nov 6, 2017

+1

@mitchhankins01

This comment has been minimized.

Copy link

commented Nov 8, 2017

+1 same issue

@mikelehen

This comment has been minimized.

Copy link
Member

commented Nov 9, 2017

FYI- If anybody can help isolate what's going wrong under react native on Android, that would potentially expedite the resolution of this bug. A good first step might be to capture logging with firebase.firestore.setLogLevel('debug'), though debugging through the code may also be necessary.

@firebase firebase deleted a comment Nov 9, 2017

@chrisbianca

This comment has been minimized.

Copy link

commented Nov 10, 2017

If you're happy to move away from Expo, then https://github.com/invertase/react-native-firebase has full support for Firestore on both Android and iOS already.

@harisvsulaiman

This comment has been minimized.

Copy link

commented Nov 10, 2017

@chrisbianca I have moved on to https://github.com/invertase/react-native-firebase, but some projects require the ease and features of expo and where performance is not a problem.

@clarktank

This comment has been minimized.

Copy link

commented Nov 13, 2017

If you are interested in Firestore vote here. https://expo.canny.io/feature-requests/p/adding-react-native-firebase-to-expo

@david1820

This comment has been minimized.

Copy link

commented Nov 16, 2017

+1 Same issue.

@dmitriy-bizyaev

This comment has been minimized.

Copy link

commented Nov 16, 2017

I think i've found the cause of this bug. The problem is not in Firestore library itself, but in the underlying WebChannel client. When it tries to make a GET request, it calls XMLHttpRequest.prototype.send with an empty string, which makes it fail on Android for some reason. Here's a workaround:

const originalSend = XMLHttpRequest.prototype.send;
XMLHttpRequest.prototype.send = function(body) {
  if (body === '') {
    originalSend.call(this);
  } else {
    originalSend.call(this, body);
  }
};

Apply this hack before making any calls to Firestore.

@mikelehen

This comment has been minimized.

Copy link
Member

commented Nov 17, 2017

@dmitriy-bizyaev Wow! Thanks for tracking this down and providing the workaround. This is super helpful.

@wenbozhu Do you think it makes sense to make this change in WebChannel itself? Or perhaps we should file a bug against the React Native XMLHttpRequest implementation for Android...

@harisvsulaiman

This comment has been minimized.

Copy link

commented Nov 17, 2017

@mikelehen we could file an issue and see what they have to say, if its a breaking change ( i doubt it ),we can patch it here.

@wenbozhu

This comment has been minimized.

Copy link

commented Nov 17, 2017

if xhr.send("") works on iOS, then we should file a bug for Andriod React Native.

===

I would also like to see a patch in closure to avoid calling xhr.send("") for GET.

@tyoshino does the xhr2 api spec say anything about passing "" vs "undefined" body-content for a GET?

@mikelehen

This comment has been minimized.

Copy link
Member

commented Nov 17, 2017

If anybody's willing, I'd appreciate it if somebody could test a patched version of the Firestore SDK that should (hopefully) now work with React Native on Android.

Probably only one of these is necessary, but to be safe:

  1. Copy https://gist.githubusercontent.com/mikelehen/2d36ed9ce5a93c879670f1eaa398ad48/raw/23576d4a5e0cad6c101d29278bc801481992e015/firebase-firestore.js over your node_modules/firebase/firebase-firestore.js
  2. Copy https://gist.githubusercontent.com/mikelehen/444950a35019208f3aaf18d37ab4937c/raw/c66d06fa0dcd03f353353deedcac7b5b9090d153/index.js over your node_modules/@firebase/webchannel-wrapper/dist/index.js

This will let us confirm that the change to WebChannel to avoid calling xhr.send('') does indeed resolve the issue. Thanks!

@clarktank

This comment has been minimized.

Copy link

commented Nov 17, 2017

@mikelehen I have replaced the two files but I am still getting the same error.
I tried to go through the code, but couldn't find @dmitriy-bizyaev's change. Can you show us on github what part of the file you have changed?

@clarktank

This comment has been minimized.

Copy link

commented Nov 17, 2017

[exp] Firestore (4.6.1) 2017-11-17T17:31:28.977Z [MemoryPersistence]: Starting transaction: Start LocalStore
[exp] Firestore (4.6.1) 2017-11-17T17:31:28.979Z [MemoryPersistence]: Starting transaction: Get last stream token
[exp] Firestore (4.6.1) 2017-11-17T17:31:28.980Z [MemoryPersistence]: Starting transaction: Get next mutation batch
[exp] Firestore (4.6.1) 2017-11-17T17:31:28.985Z [MemoryPersistence]: Starting transaction: Allocate query
[exp] Firestore (4.6.1) 2017-11-17T17:31:28.987Z [MemoryPersistence]: Starting transaction: Execute query
[exp] Firestore (4.6.1) 2017-11-17T17:31:28.989Z [MemoryPersistence]: Starting transaction: Remote document keys
[exp] Firestore (4.6.1) 2017-11-17T17:31:28.995Z [Connection]: Creating WebChannel: https://firestore.googleapis.com/google.firestore.v1beta1.Firestore/Listen/channel [object Object]
[exp] Firestore (4.6.1) 2017-11-17T17:31:29.006Z [Connection]: Opening WebChannel transport.
[exp] Firestore (4.6.1) 2017-11-17T17:31:29.008Z [Connection]: WebChannel sending: {"database":"projects/expo-sample/databases/(default)","addTarget":{"query":{"structuredQuery":{"from":[{"collectionId":"reviews"}],"orderBy":[{"field":{"fieldPath":"__name__"},"direction":"ASCENDING"}]},"parent":"projects/expo-sample/databases/(default)"},"targetId":2}}
[exp] Firestore (4.6.1) 2017-11-17T17:31:29.153Z [Connection]: WebChannel transport opened.
@mikelehen

This comment has been minimized.

Copy link
Member

commented Nov 17, 2017

Thanks for checking @clarktank. The actual code change is in the closure-library code included via our dependency on closure-builder and ends up minified here, so it's a bit hard to highlight the change, but basically, this:

F(this.b,U(this,"Sending request")),this.o=!0,this.a.send(t),this.o=!1

changed to:

F(this.b,U(this,"Sending request")),this.o=!0,void 0!==n||"GET"!==e?this.a.send(t):this.a.send(),this.o=!1

in the files I posted (depending which file you look at the exact variable names might be different, but you can search for 'Sending request' to find the section).

If you can verify that the updated code is getting hit but still not working, that would be very useful to know. Else, we'll have to get a local repro set up and debug through it, but I'm not sure when that will happen.

@wenbozhu

This comment has been minimized.

Copy link

commented Nov 17, 2017

The closure change makes sure that for a GET, send() will be called instead of send(''). Isn't send('') the root cause of this issue?

===

Closure calls send('') when body is unspecified mostly to force the C-L header for formdata.

@clarktank

This comment has been minimized.

Copy link

commented Nov 17, 2017

Here is how to setup Expo/React-native. It took me about 7 min.
I have created a file that fetches from firestore database.
Create a virtual machine like Ubuntu:

  1. sudo npm install -g exp
  2. go to your workspace directory and put sudo exp init test-project
  3. cd in to the test-project folder and enter npm install --save firebase
  4. replace the App.js file in the test-project directory with this code https://github.com/clarktank/firestore-for-expo-team/blob/master/App.js
  5. then in the test-project directory enter sudo exp start --tunnel
  6. scan the QR code from your phone.
  7. you can see the console log in the terminal that you entered sudo exp start --tunnel
@samsafay

This comment has been minimized.

Copy link
Author

commented Nov 17, 2017

I solved the problem.
Change this, on line 2339:
void 0 !== c || "GET" !== b ? this.a.send(a) : this.a.send(),
with
void 0 != c || "GET" !== b ? this.a.send(a) : this.a.send(),

in the file node_modules\@firebase\webchannel-wrapper\dist\index.js
How to see that file correctly? Get prettier package on VS Code and press alt+shif+f.

To get rid of the runtime error, change:
const MAX_TIMER_DURATION_MS = 60 * 1000;
to
const MAX_TIMER_DURATION_MS = 6000 * 1000;

in your node_modules\react-native\Libraries\Core\Timers\JSTimers.js

@mikelehen

This comment has been minimized.

Copy link
Member

commented Nov 17, 2017

@samsafay Thanks! That is very helpful. I'll sync up with folks internally to try to get the original fix reworked.

@josealfonsoweb

This comment has been minimized.

Copy link

commented Nov 19, 2017

Hey @samsafay I tried to do the changes, but I get the same error, Maybe I did anything wrong...can you help me to get this file fixed? how can I download it? will this allow me to use firestore in React Native with EXPO?

@josealfonsoweb

This comment has been minimized.

Copy link

commented Nov 19, 2017

Hey mikelehen @mikelehen I think you already found the solution... When will This issue be resolved? I want to use firestore in React Native with EXPO

@samsafay

This comment has been minimized.

Copy link
Author

commented Nov 19, 2017

https://gist.github.com/samsafay/810037a1df791f3d0fab1d92a70aabd2
Replace this file with your node_modules/@firebase/webchannel-wrapper/dist/index.js
Make sure you copy paste from the raw.

Don't confuse this with the original fix that mikelehen sent. The original fix didn't work and needed to be modified a little. The new fix is highlighted here: https://gist.github.com/samsafay/810037a1df791f3d0fab1d92a70aabd2#file-index-js-L2339
Now, with this fix, you can use Firestore with Expo or wait for Google to internally patch it and release it in their next version of Firebase.

If you are getting a runTime error you can fix that by changing

const MAX_TIMER_DURATION_MS = 60 * 1000;
to
const MAX_TIMER_DURATION_MS = 6000 * 1000;

in your node_modules\react-native\Libraries\Core\Timers\JSTimers.js

@mikelehen

This comment has been minimized.

Copy link
Member

commented Dec 19, 2017

Per my previous comment about the fix on our end having to be rolled back, I think we need to try to get React Native fixed. I looked a bit through the code and I suspect the problem is here: https://github.com/facebook/react-native/blob/cb49877a25bac9e72de85f6703eba7f07a308061/ReactAndroid/src/main/java/com/facebook/react/modules/network/NetworkingModule.java#L238

If it falls into the else block then I think contentType will be null and it'll hit an error. I think the fix is probably to change the if (data == null) { line to

// The spec (https://xhr.spec.whatwg.org/#dom-xmlhttprequest-send) defines GET and HEAD to be treated as if the body was null.
if (data == null || method.toLowerCase() === 'get' || method.toLowerCase() === 'head') {

If anybody wants to try to verify this and get a PR submitted to React Native, it would be much appreciated. Else, I'll just log a bug against React Native in the next few days.

@mikelehen

This comment has been minimized.

Copy link
Member

commented Jan 2, 2018

Anybody willing to create a react-native PR based on my comment above? :-)

@mikelehen

This comment has been minimized.

Copy link
Member

commented Jan 2, 2018

I also have a suggested fix for the timer warning (facebook/react-native#17083) which somebody could submit a PR for. :-)

samsafay added a commit to samsafay/react-native that referenced this issue Jan 3, 2018

Checking For the GET or HEAD in send method
The following change is suggested by a Firebase member here. firebase/firebase-js-sdk#283
It follows the whatwg.org's send method guidelines. https://xhr.spec.whatwg.org/#dom-xmlhttprequest-send
At the section 3 of 4.5.6, it is suggests to check for the GET or HEAD methods, for every XMLHttpRequest send() method, then set the body to null if it is true.
@samsafay

This comment has been minimized.

Copy link
Author

commented Jan 4, 2018

@mikelehen I have a great news, the solution works like a charm! Lets hope Facebook accepts it...

@mikelehen

This comment has been minimized.

Copy link
Member

commented Jan 4, 2018

Thanks! I see though that my fix had syntax errors (because I forgot it's java). Agh. Sorry!

@NickDelfino

This comment has been minimized.

Copy link

commented Feb 11, 2018

Has there been any official closure on this? Are the fixes in the various SDKs or is it still best to edit the files manually? I am having these same issues.

@samsafay

This comment has been minimized.

Copy link
Author

commented Feb 11, 2018

This is not a SDK problem, the problem is on React-Native Android implementaion of XMLHttpRequest.
Off topic, based on personal experience, you are better off using react-native-firebase since it doesn't engage the Javascript thread; therefore a better performance.

@NickDelfino

This comment has been minimized.

Copy link

commented Feb 11, 2018

Yeah I am looking at that currently. Trying to keep everything in pure Expo but may not be possible right now. Some of the suggested spot fixes work but I am worried about downstream consequences. Are the PRs just in limbo right now? I hope they can get in soon.

Thanks for all your work in regards to this issue.

@samsafay

This comment has been minimized.

Copy link
Author

commented Feb 11, 2018

Your welcome. Pure Expo, it might be possible in 2019 but not now!

@TaikiFnit TaikiFnit referenced this issue Feb 17, 2018

Closed

Remove Expo #8

facebook-github-bot added a commit to facebook/react-native that referenced this issue Mar 7, 2018

check for GET and Head in send request
Summary:
React Native had an underlying problem connecting to Firestore (Google's latest database) from Android devices. You can follow the issue [here](firebase/firebase-js-sdk#283).
The main problem was in NetworkingModule.java. Please refer to section 3 of 4.5.6 in whatwg.org's guideline https://xhr.spec.whatwg.org/#dom-xmlhttprequest-send

In this [video](https://www.youtube.com/watch?v=tILagf46ys8), I am showing how the react native behaved before adding the new fix and how it worked after the new fix added.  The new fix starts at 50 seconds.

[ANDROID] [BUGFIX] [FIRESTORE][XMLHttpRequest][ReactAndroid/src/main/java/com/facebook/react/modules/network/NetworkingModule.java] - Fixes the connection to Firestore by following whatwg.org's XMLHttpRequest send() method
Closes #17940

Differential Revision: D7173468

Pulled By: hramos

fbshipit-source-id: 354d36f03d611889073553b93a7c43c6d4363ff3

@dv297 dv297 referenced this issue Mar 8, 2018

Closed

setTimeout Warning #143

@mikelehen

This comment has been minimized.

Copy link
Member

commented Mar 15, 2018

Just to keep this updated, it looks like @samsafay's PR (facebook/react-native#17940) was committed on mar 6th (facebook/react-native@d52569c), and it should be included in the next react-native release... though I don't know when that'll be.

@samtstern

This comment has been minimized.

Copy link
Member

commented Mar 15, 2018

@mikelehen is there any action left from our perspective or should we close this and point people over to react-native to watch the progress?

@mikelehen

This comment has been minimized.

Copy link
Member

commented Mar 15, 2018

@samtstern No action for us... Folks can likely watch https://github.com/facebook/react-native/releases and wait for a release newer than the "February 2018" one. At that point, I believe the issue with Android not working should be fixed.

@i2chris

This comment has been minimized.

Copy link

commented Mar 21, 2018

@mikelehen thanks!!

ide added a commit to expo/react-native that referenced this issue Mar 21, 2018

check for GET and Head in send request
Summary:
React Native had an underlying problem connecting to Firestore (Google's latest database) from Android devices. You can follow the issue [here](firebase/firebase-js-sdk#283).
The main problem was in NetworkingModule.java. Please refer to section 3 of 4.5.6 in whatwg.org's guideline https://xhr.spec.whatwg.org/#dom-xmlhttprequest-send

In this [video](https://www.youtube.com/watch?v=tILagf46ys8), I am showing how the react native behaved before adding the new fix and how it worked after the new fix added.  The new fix starts at 50 seconds.

[ANDROID] [BUGFIX] [FIRESTORE][XMLHttpRequest][ReactAndroid/src/main/java/com/facebook/react/modules/network/NetworkingModule.java] - Fixes the connection to Firestore by following whatwg.org's XMLHttpRequest send() method
Closes facebook#17940

Differential Revision: D7173468

Pulled By: hramos

fbshipit-source-id: 354d36f03d611889073553b93a7c43c6d4363ff3

ide added a commit to facebook/react-native that referenced this issue Mar 21, 2018

check for GET and Head in send request
Summary:
React Native had an underlying problem connecting to Firestore (Google's latest database) from Android devices. You can follow the issue [here](firebase/firebase-js-sdk#283).
The main problem was in NetworkingModule.java. Please refer to section 3 of 4.5.6 in whatwg.org's guideline https://xhr.spec.whatwg.org/#dom-xmlhttprequest-send

In this [video](https://www.youtube.com/watch?v=tILagf46ys8), I am showing how the react native behaved before adding the new fix and how it worked after the new fix added.  The new fix starts at 50 seconds.

[ANDROID] [BUGFIX] [FIRESTORE][XMLHttpRequest][ReactAndroid/src/main/java/com/facebook/react/modules/network/NetworkingModule.java] - Fixes the connection to Firestore by following whatwg.org's XMLHttpRequest send() method
Closes #17940

Differential Revision: D7173468

Pulled By: hramos

fbshipit-source-id: 354d36f03d611889073553b93a7c43c6d4363ff3
@Luckygirlllll

This comment has been minimized.

Copy link

commented Apr 4, 2018

@dmitriy-bizyaev Where exactly in the project should I put that lines of the code which you wrote?

@dmitriy-bizyaev

This comment has been minimized.

Copy link

commented Apr 4, 2018

@Luckygirlllll Put it before your firebase.initializeApp() call.

@mikelehen

This comment has been minimized.

Copy link
Member

commented Apr 24, 2018

FYI- The original issue with xhr.send() on Android that this bug was tracking has been resolved in the March React Native release (0.55.0).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.