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

Refactor and cleanup for version 11. #117

Merged
merged 11 commits into from
Jan 18, 2023
Merged

Conversation

gagik
Copy link
Contributor

@gagik gagik commented Dec 30, 2022

This is a rather large refactor that mainly does the following:

General:

  • Fixes ESLint / prettier lint / typescript errors.
  • Introduces more type safety in different parts of the code and refactors bits and pieces where types or other conflicts were not handled correctly.

/realm-flipper-plugin-device - Device library

  • Improves the way listeners work with regards to additions/deletions/modifications. Listener.ts => PluginConnectedObjects.ts
  • _objectKey field being used for serialized objects renamed to _pluginObjectKey to avoid conflicts with Realm function but should likely be changed completely in future implementation. See #86 and #121.

/flipper-plugin-realm - Desktop plugin

  • Adds realm as a dev dependency so the underlying types used by the desktop plugin extend Realm classes (thus making it easier to notice i.e. breaking changes or other conflicts in future releases).

/testApp - Plugin test application

  • Now built on top of the Realm TS template app using a simple drop-in replacement, making it easier to update to future versions. All new additions are reflected in metro config, package.json, index.ts and testApp/flipperTest directory, the rest is just regular template code.
  • Added metro config to make sure it uses the local device library implementation.
  • Has both legacy tests as well as ability to test with the template ToDo app.

@cla-bot cla-bot bot added the cla: yes label Dec 30, 2022
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.

Just read through a bit of the code. I'll give this a more thorough review when it's marked as ready. But really, awesome work so far. I think the community will really appreciate this, and hopefully also start making their own PRs for it.

Copy link
Contributor Author

@gagik gagik left a comment

Choose a reason for hiding this comment

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

Just adding some comments while those are fresh on my mind.

flipper-plugin-realm/src/CommonTypes.tsx Show resolved Hide resolved
flipper-plugin-realm/.prettierrc Show resolved Hide resolved
@gagik gagik marked this pull request as ready for review January 6, 2023 14:53
@gagik gagik force-pushed the gagik/typescriptrefactoring branch from 8502cd3 to 3f036e3 Compare January 12, 2023 14:10
@gagik gagik force-pushed the gagik/typescriptrefactoring branch from 3f036e3 to fdd81eb Compare January 12, 2023 14:29
.eslintrc Show resolved Hide resolved
flipper-plugin-realm/package.json Show resolved Hide resolved
flipper-plugin-realm/src/CommonTypes.tsx Show resolved Hide resolved
flipper-plugin-realm/src/utils/Renderer.tsx Show resolved Hide resolved
@@ -42,8 +42,8 @@
"realm": ">=10.0.0"
},
"dependencies": {
"react-native-flipper": ">=0.162.0",
"realm": ">=10.20.0"
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 am removing realm as a dependency... pretty sure all of this is meant to be peer dependencies rather than dependencies and I am trying to minimize unnecessary ones. I think react-native-flipper can be moved as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, these should all be peerDependencies

@gagik gagik requested a review from kraenhansen January 12, 2023 16:10
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.

Very nice work. Thanks for the tour of the flipper code :)
I have a few comments/suggestions/questions, but nothing holding this back.

flipper-plugin-realm/package.json Show resolved Hide resolved
flipper-plugin-realm/src/components/DataTable.tsx Outdated Show resolved Hide resolved
flipper-plugin-realm/src/components/DataTable.tsx Outdated Show resolved Hide resolved
flipper-plugin-realm/src/components/DataTable.tsx Outdated Show resolved Hide resolved
@@ -42,8 +42,8 @@
"realm": ">=10.0.0"
},
"dependencies": {
"react-native-flipper": ">=0.162.0",
"realm": ">=10.20.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, these should all be peerDependencies

realm-flipper-plugin-device/src/ConvertFunctions.tsx Outdated Show resolved Hide resolved
realm-flipper-plugin-device/src/PluginConnectObjects.ts Outdated Show resolved Hide resolved
realm-flipper-plugin-device/src/PluginConnectObjects.ts Outdated Show resolved Hide resolved
@gagik gagik merged commit 14f00a0 into main Jan 18, 2023
@gagik gagik deleted the gagik/typescriptrefactoring branch 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
Development

Successfully merging this pull request may close these issues.

2 participants