-
Notifications
You must be signed in to change notification settings - Fork 13
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/messages #106
Refactor/messages #106
Conversation
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.
Overall, I love the direction @josefinalliende! And the tests! 🔥
One major question and a small nitpick.
import { eventToReaction } from '$lib/utils/reaction'; | ||
import { eventToDeletion } from '$lib/utils/deletion'; | ||
|
||
export function createChatStore() { |
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.
Curious about why you chose to use a store here instead of a Class. No judgement, I'm not super clear myself on why one would want to use one or the other... I think I'm using both in other parts of the app (toasts are a class, accounts are a store) and I feel like we should pick one pattern and use that unless we have a good reason otherwise. In any case, maybe you understand this better than I do so would love to hear your thoughts.
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.
To be honest, coming from a Ruby background, I love classes. However, from what I’ve learned, Svelte takes a more functional approach rather than an object-oriented one, with store functions and runes for reactivity. In this particular case, the advantage of using a Svelte store is that they are are inherently reactive. If we used a class, we would need to manually implement the subscription logic. (In the page we have suscriptions to the chat sotre when it is used with a $ , for example $chatStore.messages will be automatically updated when any change happens in the internal messageMap of the chat store).
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.
I guess we could get something similar using $state fields inside a class but it's more to implement ourselves. This looks great as is.
|
||
// Add messages to the chat store | ||
chatStore.clear(); | ||
events.forEach(event => { |
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.
nitpick but I'm starting to slowly try to switch out all the forEach
loops for for..of
- better performance
- more predictable execution with async ops
for (const event of events) {
chatStore.handleEvent(event);
}
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.
Oh thanks! I had no idea that it was better. I'll change it!
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.
Quick overview courtesy of Claude. :)
Here are some TypeScript examples comparing forEach
with recommended alternatives:
1. Basic iteration with side effects
// Using forEach (not recommended for simple iteration)
const numbers: number[] = [1, 2, 3, 4, 5];
numbers.forEach(num => {
console.log(num * 2);
});
// Better: Using for...of (cleaner and more flexible)
for (const num of numbers) {
console.log(num * 2);
if (num > 3) break; // Can exit early if needed
}
2. Transforming data
// Using forEach with side effects (not recommended)
const numbers: number[] = [1, 2, 3, 4, 5];
const doubled: number[] = [];
numbers.forEach(num => {
doubled.push(num * 2);
});
// Better: Using map (functional, clearer intent)
const doubledBetter: number[] = numbers.map(num => num * 2);
3. Filtering data
// Using forEach with side effects (not recommended)
const numbers: number[] = [1, 2, 3, 4, 5];
const evens: number[] = [];
numbers.forEach(num => {
if (num % 2 === 0) {
evens.push(num);
}
});
// Better: Using filter (functional, clearer intent)
const evensBetter: number[] = numbers.filter(num => num % 2 === 0);
4. Async operations
// Using forEach with async (PROBLEMATIC - doesn't wait for promises)
const userIds: number[] = [1, 2, 3];
userIds.forEach(async (id) => {
const userData = await fetchUser(id);
console.log(userData); // These will execute in unpredictable order
});
// Better: Using for...of with async/await
async function processUsers() {
for (const id of userIds) {
const userData = await fetchUser(id);
console.log(userData); // These will execute in sequence
}
}
5. Performance-critical code
// Using forEach (slower for large arrays)
function sumArray(numbers: number[]): number {
let sum = 0;
numbers.forEach(num => {
sum += num;
});
return sum;
}
// Better: Using traditional for loop (faster)
function sumArrayFaster(numbers: number[]): number {
let sum = 0;
for (let i = 0; i < numbers.length; i++) {
sum += numbers[i];
}
return sum;
}
// Alternative: Using reduce (functional but still faster than forEach)
function sumArrayReduce(numbers: number[]): number {
return numbers.reduce((sum, num) => sum + num, 0);
}
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.
Thanks for the explanation! Now I've replaced them
162f2a4
to
1d5834c
Compare
Context
While working on issue #70, I realized that the current deletion logic couldn’t be reused for reactions. The existing approach checks all events for each message to find reactions and then repeats the process to look for deletions. Adding deletion handling on top of that would have led to a lot of unnecessary checks.
To improve this, I adjusted how events are processed, making it easier to handle reaction deletions more efficiently. I also moved this logic out of the chat page, which was already quite complex. This keeps the chat component focused on handling messages, reactions, and payments without needing to deal with Nostr events directly for messages, while also making the logic easier to test.
What has been done:
Introduced a new store, chatStore, to structure and store processed Nostr events. To support this, new types were added to make handling events easier for the chat page. When Nostr events are processed, they are mapped into these types, allowing the page to work with structured data rather than raw events.
Here are the key new types:
Message
: Represents processed kind 9 events. Depending on their tags, they may include aLightningInvoice
or aLightningPayment
. Messages can also have an array of Reactions.Reaction
: Represents kind 7 events.Deletion
: Represents kind 5 events, which can currently target messages and reactions.Additionally, the way deletions are handled was improved. Instead of checking all events to determine if a message or reaction has been deleted, deletions are now mapped by their target event ID. This allows for direct lookups, making the process more efficient.
Commit by commit:
What to check:
What's next
Some ideas for the future...
PS: Sorry about he length of the PR, next ones will be shorter!