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

[WEB SDK COMPAT] await .push() returns null, but works on web sdk #893

Closed
brunolemos opened this issue Mar 19, 2018 · 7 comments · Fixed by #1619
Closed

[WEB SDK COMPAT] await .push() returns null, but works on web sdk #893

brunolemos opened this issue Mar 19, 2018 · 7 comments · Fixed by #1619
Assignees
Labels
help: needs-tests Needs a test case adding to the project tests. impact: web-implementation An API in RNFB does not conform to the Firebase Web SDK platform: javascript plugin: database Firebase Realtime Database type: bug New bug report
Milestone

Comments

@brunolemos
Copy link
Contributor

brunolemos commented Mar 19, 2018

Issue

Given the code:

const messageRef = await firebase.database().ref('xxx/123/messages').push(message)
// messageRef = null

If I use await it returns null. But on web sdk it works just fine.
Just converted from firebase to react-native-firebase and faced this compatibility problem.
If I don't use await it works fine, but it should match the web.

Related but closed: #147, #696

Environment

iOS (not tested on Android)

  1. Application Target Platform: macOS High Sierra
  1. Development Operating System: iOS 11
  1. Build Tools: ?
  1. React Native version: 0.54
  1. RNFirebase Version: 3.3.1
  1. Firebase Module: Database
@Salakar Salakar added impact: web-implementation An API in RNFB does not conform to the Firebase Web SDK help: needs-triage Issue needs additional investigation/triaging. platform: javascript labels Mar 20, 2018
@Salakar Salakar added this to the v4.0.0 Release milestone Mar 26, 2018
@Salakar Salakar added the help: needs-tests Needs a test case adding to the project tests. label Mar 26, 2018
@Salakar Salakar modified the milestones: v4.3.0 Release, v5.0.0 Release Jul 9, 2018
@Friis1978
Copy link

Friis1978 commented Aug 9, 2018

@Salakar Anything new on this issue, still does not work for me.

const ref = firebase.database().ref(/users/${uid}/points).push({ name: propValues.name, address: propValues.address, number: propValues.number, }) .then(() => { const key = ref.key; console.log(key); }) .catch((error) => { console.log(error); });

This should give me the key value, but is empty.

@Friis1978
Copy link

Hmmm It actually worked without the then, so the problem is solved in my case.

Just thought that the reference key was based on what the promise returned, so I was using the 'then' to wait for that promise.

This is the answer:
const ref = firebase.database().ref(dbref).push({ name: propValues.name, address: propValues.address, }); console.log(ref.key);

@Salakar
Copy link
Member

Salakar commented Aug 9, 2018

@Friis1978 without the await is also correct as creating a new reference + id path via push() is synchronous/client sided, though I believe the web SDK supports await as well like you mentioned above so we still need to add support for that our end.

Are you able to test the following 3 scenarios on the Web SDK if you still have it handy and report back with the logs if possible, would be super helpful:

// 1
const ref1 = firebase.database().ref(dbref).push();
console.log('1a', ref1.key); // should be key ?
const result = await ref1.set({ name: propValues.name, address: propValues.address });
console.log('1b', result); // should be null ?

// 2
const ref2 = await firebase.database().ref(dbref).push({ name: propValues.name, address: propValues.address });
console.log('2a', ref2.key); // should be key ?

// 3
const ref3 = await firebase.database().ref(dbref).push();
console.log('3a', ref3.key); // should be key ?
const result = await ref3.set({ name: propValues.name, address: propValues.address });
console.log('3b', result); // should be null ?

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

@stale
Copy link

stale bot commented Sep 2, 2018

This issue has been automatically marked as stale because it has not had recent user activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Type: Stale Issue has become stale - automatically added by Stale bot label Sep 2, 2018
@simonbengtsson
Copy link
Contributor

Results with firebase 5.4.2:

1a -LLuT3yPEkWQYq7JinLP
1b undefined
2a -LLuT4A5DPPpDSjEaGg-
3a -LLuT4CbmLlncywg1l3e
3b undefined

@stale stale bot removed the Type: Stale Issue has become stale - automatically added by Stale bot label Sep 8, 2018
@Salakar Salakar added the Workflow: Needs Review Pending feedback or review from a maintainer. label Sep 8, 2018
@Salakar
Copy link
Member

Salakar commented Sep 8, 2018

@simonbengtsson awesome, thanks for that - looks like await support on push() is missing on RNFB - will get it fixed.

@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 closed this as completed 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. impact: web-implementation An API in RNFB does not conform to the Firebase Web SDK platform: javascript plugin: database Firebase Realtime Database type: bug New bug report
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants