-
Notifications
You must be signed in to change notification settings - Fork 170
Unify and simplify actor and RPC message types #1462
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
Conversation
#747 Bundle Size — 1.5MiB (~-0.01%).Warning Bundle contains 13 duplicate packages – View duplicate packages Bundle metrics
Bundle size by type
Bundle analysis report Branch jerel/simplify-types Project dashboard |
.changeset/six-months-teach.md
Outdated
"apollo-client-devtools": patch | ||
--- | ||
|
||
Create a universal message bridge to handle both actor and rpc messages. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to move this to the other PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a good improvement :)
src/extension/actor.ts
Outdated
if (!listeners) { | ||
listeners = new Set(); | ||
messageListeners.set(name, listeners as Set<(message: Messages) => void>); | ||
messageListeners.set( | ||
name, | ||
listeners as Set<(message: ActorMessage) => void> | ||
); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (!listeners) { | |
listeners = new Set(); | |
messageListeners.set(name, listeners as Set<(message: Messages) => void>); | |
messageListeners.set( | |
name, | |
listeners as Set<(message: ActorMessage) => void> | |
); | |
} | |
if (!listeners) { | |
listeners = new Set(); | |
messageListeners.set(name, listeners); | |
} |
Seems the cast is not neccessary here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch!
2d2a7d1
to
4c9d938
Compare
ab35992
to
c3377e5
Compare
8115beb
to
2b8ead6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love how this just keeps getting further simplified! 🚀
I saw an opportunity to simplify the message types in the app. We use a ton of generics with both the actor and RPC models, but looking through this more, this seemed largely unnecessary because we don't have messages going a bunch of different ways to different things. We largely want to create a single actor/rpc client and use that for messaging in a specific area of the extension.
As a part of this, I decided to simplify the types and remove the need for generics. I've combined the message types together into a single union type that has now moved to the respective files where the implementation lives. This should make it easier to know both where to put the type and avoid confusion on what message type should include new types.