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(firestore): accept nested undefined in arrays if ignoreUndefined set #5527

Merged
merged 2 commits into from
Jul 21, 2021

Conversation

mikehardy
Copy link
Collaborator

Description

The previous change to add ignoreUndefined support and operate like firebase-js-sdk (#5321) did not take into account nested undefined values in arrays, this PR repairs that

Related issues

Fixes #5437

Release Summary

2 commits with conventional commit titles, rebase merge should work

Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
    • Yes
  • My change supports the following platforms;
    • Android
    • iOS
  • My change includes tests;
    • e2e tests added or updated in packages/\*\*/e2e
    • jest tests added or updated in packages/\*\*/__tests__
  • I have updated TypeScript types that are affected by my change.
  • This is a breaking change;
    • Yes
    • No

Test Plan

I added unit and e2e tests for it, I will request testing from the affected parties via patch-package


Think react-native-firebase is great? Please consider supporting the project with any of the below:

the excluded archs as commiting here is what the currently "normal"
case will look like, but arm64 macs will allows remove the inclusion
when they do a pod install, so this value will "flap" a little. Irritating.
@vercel
Copy link

vercel bot commented Jul 17, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployments, click below or on the icon next to each commit.

react-native-firebase – ./

🔍 Inspect: https://vercel.com/invertase/react-native-firebase/AsumUginUxEeSuh5aDmfmzKH4pNt
✅ Preview: https://react-native-firebase-git-mikehardy-firestore-c1a0dd-invertase.vercel.app

react-native-firebase-next – ./website_modular

🔍 Inspect: https://vercel.com/invertase/react-native-firebase-next/4Mi8kAvPY8TvLyEVvM7R8ayz3u3M
✅ Preview: Failed

[Deployment for 56e8354 failed]

@mikehardy mikehardy added Workflow: Pending Merge Waiting on CI or similar Workflow: Waiting for User Response Blocked waiting for user response. labels Jul 17, 2021
@codecov
Copy link

codecov bot commented Jul 17, 2021

Codecov Report

Merging #5527 (d2a4a69) into master (f47a7c7) will decrease coverage by 0.02%.
The diff coverage is 72.73%.

❗ Current head d2a4a69 differs from pull request most recent head 56e8354. Consider uploading reports for the commit 56e8354 to get more accurate results

@@            Coverage Diff             @@
##           master    #5527      +/-   ##
==========================================
- Coverage   74.56%   74.54%   -0.01%     
==========================================
  Files          96       96              
  Lines        4287     4292       +5     
  Branches      921      923       +2     
==========================================
+ Hits         3196     3199       +3     
- Misses       1021     1022       +1     
- Partials       70       71       +1     

@mikehardy
Copy link
Collaborator Author

Requested feedback on linked issue but disposed to merge + release in a few days if not.
The error was immediate + obvious even at the jest level when adding unit tests for it, and the e2e test was just added for confirmation after

@mikehardy mikehardy force-pushed the @mikehardy/firestore-nested-undefined branch from ababe87 to 56e8354 Compare July 17, 2021 18:27
@mikehardy mikehardy removed Workflow: Pending Merge Waiting on CI or similar Workflow: Waiting for User Response Blocked waiting for user response. labels Jul 21, 2021
@mikehardy mikehardy merged commit 224383f into master Jul 21, 2021
@mikehardy mikehardy deleted the @mikehardy/firestore-nested-undefined branch July 21, 2021 17:36
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.

🔥 [🐛] ignoreUndefinedProperties not respected for arrays of objects
1 participant