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.

Sign up for GitHub

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

Jump to bottom

Remove null values in nested objects #353

Merged
merged 40 commits into from
Sep 28, 2023
Merged

Remove null values in nested objects #353

merged 40 commits into from
Sep 28, 2023

Conversation

chrispader
Copy link
Contributor

@chrispader chrispader commented Sep 19, 2023 •
edited
Loading

@tgolen @blazejkustra @marcaaron @thienlnam

Details

At the moment, we only remove nullish values (undefined and null) for top-level keys in objects when merging.

In this Slack thread we agreed to also remove nullish values from nested objects.

Related Issues

#301

Automated Tests

Linked PRs

This was referenced Sep 19, 2023
Merged
Closed
tgolen
tgolen previously approved these changes Sep 20, 2023
View reviewed changes
Copy link
Collaborator

@tgolen tgolen left a comment

Choose a reason for hiding this comment

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

I approve this PR. Let's merge as-is.

@marcaaron
Copy link
Contributor

I think it needs some more work, no?

@chrispader
Copy link
Contributor Author

chrispader commented Sep 20, 2023 •
edited
Loading

I approve this PR. Let's merge as-is.

LMAO! 😂 still not sure if it's a joke though.

I think it needs some more work, no?

Yes, it needs more work, just added the TODO comment so far...

@tgolen
Copy link
Collaborator

tgolen commented Sep 21, 2023

All tests are passing and there are no performance regressions. I think we're good here 👍

@chrispader chrispader changed the title Remove nullish values in nested objects Remove null values in nested objects Sep 21, 2023
@chrispader chrispader changed the title Remove null values in nested objects [HOLD on #359] Remove null values in nested objects Sep 21, 2023
@chrispader chrispader marked this pull request as ready for review September 21, 2023 12:45
@chrispader chrispader requested a review from a team as a code owner September 21, 2023 12:45
@melvin-bot melvin-bot bot requested review from Gonals and removed request for a team September 21, 2023 12:45
Szymon20000
Szymon20000 reviewed Sep 27, 2023
View reviewed changes
lib/utils.js Outdated Show resolved Hide resolved
Szymon20000
Szymon20000 reviewed Sep 27, 2023
View reviewed changes
lib/utils.js Outdated Show resolved Hide resolved
Szymon20000
Szymon20000 approved these changes Sep 27, 2023
View reviewed changes
Copy link
Contributor

@Szymon20000 Szymon20000 left a comment

Choose a reason for hiding this comment

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

Looks good!
I would remove lodash from the fast merge function. That kills Hermes optimisations and we merge quite frequently.

@chrispader
Copy link
Contributor Author

chrispader commented Sep 27, 2023 •
edited
Loading

Looks good!
I would remove lodash from the fast merge function. That kills Hermes optimisations and we merge quite frequently.

@marcaaron @tgolen i'll let you guys decide if we want to omit underscore here. Looks like the better approach to remove underscore

@fabioh8010 fabioh8010 mentioned this pull request Sep 27, 2023
Merged
59 tasks
@blazejkustra
Copy link
Contributor

This PR is blocking some of the TS migration issues (example). Any idea when we can expect this PR to be merged? @chrispader

@chrispader
Copy link
Contributor Author

@marcaaron @tgolen feel free to merge this whenever you think it's ready 👍

Just removed the underscore where possbile, so this should be good to go

@marcaaron
Copy link
Contributor

decide if we want to omit underscore here

Sounds good to me 👍

marcaaron
marcaaron requested changes Sep 28, 2023
View reviewed changes
Copy link
Contributor

@marcaaron marcaaron left a comment

Choose a reason for hiding this comment

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

noticed a small typo otherwise LGTM 👍

lib/Onyx.js Outdated Show resolved Hide resolved
@chrispader @marcaaron
Update lib/Onyx.js
b5a0ba8 
Co-authored-by: Marc Glasser <marc.aaron.glasser@gmail.com>
@chrispader
Copy link
Contributor Author

noticed a small typo otherwise LGTM 👍

fixed! @marcaaron

@marcaaron marcaaron merged commit 8671b9c into Expensify:main Sep 28, 2023
3 checks passed
@chrispader chrispader mentioned this pull request Sep 28, 2023
Closed
59 tasks
@ospfranco
Copy link
Contributor

Just tried to merge 1.0.96 which includes this changes and the main app is failing.

Oscar Franco Screen 000021

It also causes an issue on app start:

Screenshot_1695972930
Simulator Screenshot - iPhone SE (3rd generation) - 2023-09-29 at 09 02 10

@ospfranco ospfranco mentioned this pull request Sep 29, 2023
Merged
59 tasks
ospfranco added a commit to margelo/react-native-onyx that referenced this pull request Sep 29, 2023
@ospfranco
Revert "Merge pull request Expensify#353 from margelo/@chrispader/rem…
f3a08f7 
…ove-nested-nullish-object-keys"

This reverts commit 8671b9c, reversing
changes made to ce35c4e.
@ospfranco ospfranco mentioned this pull request Sep 29, 2023
Merged
marcaaron added a commit that referenced this pull request Sep 29, 2023
@marcaaron
Merge pull request #378 from margelo/osp/revert-remove-nested-nullish…
9c2b4e7 
…-object-keys

Revert pull request #353 remove-nested-nullish-object-keys
@marcaaron
Copy link
Contributor

Sounds good. I merged the revert.

chrispader pushed a commit to margelo/react-native-onyx that referenced this pull request Sep 29, 2023
Revert "Revert "Merge pull request Expensify#353 from margelo/@chrisp…
69c11fa 
…ader/remove-nested-nullish-object-keys""

This reverts commit f3a08f7.
@chrispader chrispader mentioned this pull request Oct 2, 2023
Merged
@fabioh8010 fabioh8010 mentioned this pull request Oct 2, 2023
Merged
@chrispader chrispader mentioned this pull request Oct 5, 2023
Merged
59 tasks
@ospfranco ospfranco mentioned this pull request Nov 10, 2023
Closed
6 tasks
@paultsimura paultsimura mentioned this pull request Mar 12, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tgolen tgolen tgolen left review comments

@fabioh8010 fabioh8010 fabioh8010 left review comments

@blazejkustra blazejkustra blazejkustra requested changes

@Szymon20000 Szymon20000 Szymon20000 approved these changes

@marcaaron marcaaron marcaaron approved these changes

@neil-marcellini neil-marcellini neil-marcellini left review comments

@Gonals Gonals Awaiting requested review from Gonals

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@chrispader @marcaaron @tgolen @neil-marcellini @blazejkustra @fabioh8010 @ospfranco @Szymon20000 @OSBotify @janicduplessis