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

Returning Reference instance from async await method causes an exception. #1464

Closed
keshavkaul opened this issue Sep 6, 2018 · 8 comments
Closed
Assignees
Labels
help: needs-tests Needs a test case adding to the project tests. platform: javascript plugin: database Firebase Realtime Database
Milestone

Comments

@keshavkaul
Copy link

keshavkaul commented Sep 6, 2018

Issue

Below code writes 'Failure' to the console:

const fetchRef = async (todoId) => {
    const ref = database()
        .ref('todos')
        .child(todoId);
    
    await ref.onDisconnect().cancel();
    await ref.onDisconnect().remove();
    return ref;
};

fetchRef('123')
    .then(ref => console.log('Success', ref))
    .catch(err => console.log('Failure', err));

Environment

  1. Application Target Platform:

Both (iOS, Android)

  1. Development Operating System:

macOS High Sierra

  1. Build Tools:

Xcode

  1. React Native version:

0.56.0

  1. React Native Firebase Version:

4.3.8

  1. Firebase Module:

database

  1. Are you using typescript?

no

  1. Stacktrace
Error: Cannot read property 'then' of undefined.
    at Reference.then (Reference.js:465)
    at tryCallTwo (core.js:45)
    at doResolve (core.js:200)
    at new Promise (core.js:66)
    at Function.Promise.resolve (es6-extensions.js:38)
    at invoke (runtime.js:168)
    at runtime.js:162
    at tryCallOne (core.js:37)
    at core.js:123
    at JSTimers.js:294

Loving react-native-firebase? Please consider supporting them with any of the below:

@keshavkaul
Copy link
Author

The issue occurs because the Reference instance has a then method and returning the reference from an async method causes the javascript environment to call the reference's then method. Inside the then method of Reference is implemented logic to throw an error if no promise is currently active.

A quick fix for this issue is to return the reference wrapped in an object.

@Ehesp Ehesp added the plugin: database Firebase Realtime Database label Sep 6, 2018
@Salakar Salakar added platform: javascript help: needs-tests Needs a test case adding to the project tests. Workflow: Needs Review Pending feedback or review from a maintainer. labels Sep 8, 2018
@Salakar Salakar added this to the v5.0.0 Release milestone Sep 8, 2018
@keshavkaul
Copy link
Author

@Salakar @Ehesp Do let me know if I can help in fixing this.

@reimertz
Copy link

I just ran intio this issue in combination with react-redux-firebase while uploading files. I assumed the issue was on my side of things but then found this issue.

@iggyfisk
Copy link

Ran into the same problem, but while creating a promise like

return new Promise((resolve, reject) => {
  resolve(firebase.database().ref());
});

results in the same Cannot read property 'then' of undefined stack trace.

@Salakar
Copy link
Member

Salakar commented Sep 20, 2018

Related to #893

Will look at getting support for this in. The issue is with Database Reference not being a real ThenableReference like the web sdk does - so needs a small bit of rework to match the web sdk.

Let's close this issue and track this on the other issue as both will be fixed after the rework and both are caused by the same underlying issue (ThenableReference).


Loving react-native-firebase and the support we provide? Please consider supporting us with any of the below:

@Salakar Salakar closed this as completed Sep 20, 2018
@secobarbital
Copy link

Actually this is a different problem: the issue here is that a simple ref has a .then method even though it has no future value, whereas #893 seems to be the opposite problem of not having .then when it should.

If you look at the Firebase JS SDK, the .then and .catch methods are only attached if there is a future value, so if we are reworking to match the JS SDK exactly, then we should be fine tracking it in the other issue.

I just tested with the latest JS SDK and confirmed it does not have this problem.

In the meantime, as a workaround, I am setting .then to null on references manually before passing it to .then-sensitive containers, like redux-saga in my case.

@Salakar
Copy link
Member

Salakar commented Sep 20, 2018

@secobarbital I don't mean they're the same problems - I mean it's the same underlying cause - ThenableReference implementation needs a rework.

You're correct though, we'd match the JS SDK.

@Salakar
Copy link
Member

Salakar commented Oct 27, 2018

Have just pushed a re-write of the push() logic to match the Web SDK.

Have added test cases from this issue and others which can be seen working here: https://github.com/invertase/react-native-firebase/blob/database-fixes/tests/e2e/database/ref/push.js#L73

This will land in v5.1.0, apologies for the delay on this one.


Loving react-native-firebase and the support we provide? Please consider supporting us with any of the below:

@Salakar Salakar modified the milestones: v5.0.0 Release, v5.1.0 Oct 27, 2018
@Salakar Salakar self-assigned this Oct 27, 2018
Salakar added a commit that referenced this issue Oct 27, 2018
 - [ANDROID] [BUGFIX] [DATABASE] - Database listeners now correctly tearing down between RN reloads. (Fixes #1498 #1611 #1609)
 - [JS] [BUGFIX] [DATABASE] - Fixed an issue where `Reference.toString()` incorrectly contains `//` instead of `/` when joining the parent and child paths.
 - [JS] [BUGFIX] [DATABASE] - Rework `.push()` behaviour to match WebSDK and correctly return a Reference instance in all scenarios. (Fixes #893 #1464 #1572)
 - [JS] [ENHANCEMENT] [UTILS] - Added a `firebase.utils().database.cleanup()` utility method which removes all database listeners.
@mikehardy mikehardy removed the Workflow: Needs Review Pending feedback or review from a maintainer. label Nov 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help: needs-tests Needs a test case adding to the project tests. platform: javascript plugin: database Firebase Realtime Database
Projects
None yet
Development

No branches or pull requests

7 participants