Skip to content

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

Merged
merged 29 commits into from
Jul 26, 2024
Merged

Conversation

jerelmiller
Copy link
Member

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.

@jerelmiller jerelmiller requested a review from a team as a code owner July 26, 2024 06:41
Copy link

relativeci bot commented Jul 26, 2024

#747 Bundle Size — 1.5MiB (~-0.01%).

36d9112(current) vs 36cd3bb main#744(baseline)

Warning

Bundle contains 13 duplicate packages – View duplicate packages

Bundle metrics  Change 3 changes Improvement 1 improvement
                 Current
#747
     Baseline
#744
Improvement  Initial JS 1.46MiB(~-0.01%) 1.46MiB
No change  Initial CSS 0B 0B
Change  Cache Invalidation 2.19% 97.56%
No change  Chunks 5 5
No change  Assets 12 12
No change  Modules 1210 1210
No change  Duplicate Modules 45 45
Change  Duplicate Code 3.08%(-0.65%) 3.1%
No change  Packages 182 182
No change  Duplicate Packages 10 10
Bundle size by type  Change 1 change Improvement 1 improvement
                 Current
#747
     Baseline
#744
Improvement  JS 1.46MiB (~-0.01%) 1.46MiB
No change  IMG 35.85KiB 35.85KiB
No change  HTML 810B 810B
No change  Other 778B 778B

Bundle analysis reportBranch jerel/simplify-typesProject dashboard

"apollo-client-devtools": patch
---

Create a universal message bridge to handle both actor and rpc messages.
Copy link
Member Author

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

Copy link
Member

@phryneas phryneas left a 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 :)

Comment on lines 83 to 86
if (!listeners) {
listeners = new Set();
messageListeners.set(name, listeners as Set<(message: Messages) => void>);
messageListeners.set(
name,
listeners as Set<(message: ActorMessage) => void>
);
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch!

@jerelmiller jerelmiller force-pushed the jerel/bridge-for-all branch from 2d2a7d1 to 4c9d938 Compare July 26, 2024 15:21
Base automatically changed from jerel/bridge-for-all to main July 26, 2024 15:27
@jerelmiller jerelmiller force-pushed the jerel/simplify-types branch from ab35992 to c3377e5 Compare July 26, 2024 15:27
@jerelmiller jerelmiller force-pushed the jerel/simplify-types branch from 8115beb to 2b8ead6 Compare July 26, 2024 16:55
Copy link
Contributor

@alessbell alessbell left a 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! 🚀

@jerelmiller jerelmiller merged commit c0b1078 into main Jul 26, 2024
9 checks passed
@jerelmiller jerelmiller deleted the jerel/simplify-types branch July 26, 2024 17:20
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.

3 participants