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

introduce FeedDetails class #105

Merged
merged 1 commit into from
Nov 10, 2022
Merged

introduce FeedDetails class #105

merged 1 commit into from
Nov 10, 2022

Conversation

staltz
Copy link
Member

@staltz staltz commented Nov 9, 2022

Context

This is not a necessary addition, it's a nice-to-have addition.

I was thinking how we return { id, parent, purpose, ... } objects in a bunch of places, and it's getting kind of out of hand. Several different files are creating/updating these objects, and they have to follow the same shape, but we're not enforcing the same shape.

Moreover, I'm also thinking that there will be many of these objects, because each branch in branchStream has many of these. It will be better to profile memory usage if we use classes, because they show up immediately in the Chrome DevTools. Also, it helps the V8 optimizer if all of the objects share exactly the same shape (same set of Object.keys() always) because then under the hood it creates a hidden struct (in C) to back all these objects.

You could say that's premature optimization, but the primary motivation here is just organization and code readability. It's a bonus that it just happens to be better for performance and memory profiling.

Solution

Introduce a new class FeedDetails that can be constructed using various helpers: fromMyMsg, fromCreateOpts, etc. Replace all the hand-written feed details objects with this class instance.

There's a tiny breaking change with tombstoned feed details: reason => tombstoneReason. I can revert that if y'all have a different opinion.

Tests pass locally, but they don't run in CI yet because this PR isn't targeting master. This PR should be merged only after #104 is merged.

@staltz staltz requested review from mixmix and arj03 November 9, 2022 20:05
Copy link
Member

@arj03 arj03 left a comment

Choose a reason for hiding this comment

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

Great. I read through this and it makes sense. I like how this moves all the message handling into 1 file. About the tombstoneReason then fine by me. Better do it now than later.

Base automatically changed from own-branches to master November 10, 2022 20:04
@staltz staltz merged commit 67341d3 into master Nov 10, 2022
@staltz staltz deleted the feed-details-class branch November 10, 2022 20:12
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.

None yet

2 participants