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

fix: *merge* setting to true irrespective of value passed #6436

Merged

Conversation

snehsagarajput
Copy link
Contributor

@snehsagarajput snehsagarajput commented Jul 31, 2022

Description

merge option is being set to true, even if false is passed! This can result in unexpected results!
As per documentations, SetOptions merge can be one of "undefined", "true" or "false". But in the current implementation the false(boolean) value is mistreated as true only!
Fixed the SetOptions merge to handle false value.

Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
    • Yes
  • My change supports the following platforms;
    • Android
    • iOS
  • This is a breaking change;
    • Yes
    • No

*merge* option is being set to true, even if false is passed! This can result in unexpected results!
@CLAassistant
Copy link

CLAassistant commented Jul 31, 2022

CLA assistant check
All committers have signed the CLA.

@vercel
Copy link

vercel bot commented Jul 31, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
react-native-firebase ✅ Ready (Inspect) Visit Preview Jul 31, 2022 at 8:22PM (UTC)
1 Ignored Deployment
Name Status Preview Updated
react-native-firebase-next ⬜️ Ignored (Inspect) Jul 31, 2022 at 8:22PM (UTC)

@snehsagarajput snehsagarajput changed the title fix(firestore,js): fixed merge ignoring false value fix(firestore, js): fixed merge ignoring false value Jul 31, 2022
@snehsagarajput snehsagarajput changed the title fix(firestore, js): fixed merge ignoring false value fix(firestore, js): fix merge ignoring false value Jul 31, 2022
@snehsagarajput snehsagarajput changed the title fix(firestore, js): fix merge ignoring false value fix(firestore): merge ignoring false value Jul 31, 2022
@snehsagarajput snehsagarajput changed the title fix(firestore): merge ignoring false value fix: merge ignoring false value Jul 31, 2022
@snehsagarajput snehsagarajput changed the title fix: merge ignoring false value fix: *merge* setting to true irrespective of value passed Jul 31, 2022
Copy link
Collaborator

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh my, that's a laughably obvious issue, really sorry that crept in the library. Thanks so much for posting a patch.

Have you tried this (via patch-package patch or similar) in production and it's working for you?

@mikehardy mikehardy added Workflow: Waiting for User Response Blocked waiting for user response. Workflow: Pending Merge Waiting on CI or similar labels Aug 1, 2022
@snehsagarajput
Copy link
Contributor Author

snehsagarajput commented Aug 4, 2022

oh my, that's a laughably obvious issue, really sorry that crept in the library. Thanks so much for posting a patch.

Have you tried this (via patch-package patch or similar) in production and it's working for you?

Haven't tried it using any library but tested it once by directly changing the library code!
@mikehardy

@mikehardy mikehardy removed the Workflow: Waiting for User Response Blocked waiting for user response. label Aug 7, 2022
@codecov
Copy link

codecov bot commented Aug 7, 2022

Codecov Report

Merging #6436 (4e6db6b) into main (b062e74) will not change coverage.
The diff coverage is 0.00%.

❗ Current head 4e6db6b differs from pull request most recent head 6015835. Consider uploading reports for the commit 6015835 to get more accurate results

@@           Coverage Diff           @@
##             main    #6436   +/-   ##
=======================================
  Coverage   25.83%   25.83%           
=======================================
  Files          97       97           
  Lines        4291     4291           
  Branches     1048     1048           
=======================================
  Hits         1108     1108           
  Misses       2583     2583           
  Partials      600      600           

@mikehardy mikehardy merged commit 85585da into invertase:main Aug 7, 2022
@mikehardy mikehardy removed the Workflow: Pending Merge Waiting on CI or similar label Aug 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants