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

Improve communication of serialised Realm objects between device and plugin #123

Merged
merged 21 commits into from
Jan 18, 2023

Conversation

gagik
Copy link
Contributor

@gagik gagik commented Jan 12, 2023

This PR...

TODO:

  • Empty collections being sent over not handled correctly
  • Embedded object modification not handled correctly
  • Test modification of different types
  • Test expanding data

@cla-bot cla-bot bot added the cla: yes label Jan 12, 2023
@gagik gagik changed the base branch from main to gagik/typescriptrefactoring January 12, 2023 13:05
@gagik gagik force-pushed the gagik/typescriptrefactoring branch from 3f036e3 to fdd81eb Compare January 12, 2023 14:29
@gagik gagik force-pushed the gagik/betterserialization branch from 2d249d7 to fe0e949 Compare January 13, 2023 16:00
@gagik gagik marked this pull request as ready for review January 16, 2023 16:50
@gagik gagik force-pushed the gagik/betterserialization branch from cae0d65 to 536e421 Compare January 17, 2023 10:26
@gagik gagik force-pushed the gagik/betterserialization branch from 536e421 to 03d88d3 Compare January 17, 2023 11:53
Copy link
Contributor

@takameyer takameyer left a comment

Choose a reason for hiding this comment

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

Really happy about all the work you've put into this. Looks like the flipper plugin is now useable!
My only suggestion is to consider breaking up "common" and "shared" and getting some of those types closer to the component using them. But this could be done later, just wanted to provide something to think about.
LGTM 🚢

}
// TODO: Fetch object with getObject to display primary key instead of objectKey
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you want to address this in a separate PR, or is this doable now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking of separate PR. although I just thought of a way to make this simpler, i.e. just send over primaryKey if it exists along with objectType / objectKey for references, so I might just do this. I generally avoid leaving TODOs like this but because of the state of the code right now this seemed like the quickest way to leave some bits for future planning.

Base automatically changed from gagik/typescriptrefactoring to main January 18, 2023 12:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants