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

UPDATE events may be missing some columns #2

Closed
ianschmitz opened this issue Oct 24, 2023 · 4 comments
Closed

UPDATE events may be missing some columns #2

ianschmitz opened this issue Oct 24, 2023 · 4 comments

Comments

@ianschmitz
Copy link

ianschmitz commented Oct 24, 2023

Hi there 👋. Thanks for creating this lib. I'm surprised something like this isn't built into Supabase yet!

I discovered an issue with the implementation here. When the updated record is received, the record is overwritten in the recordById map.

this.recordById.set(record.id, record);

This creates an issue as not all columns may be sent during the update due to the way Postgres handles large column values. You can see an existing issue over on the Supabase realtime repo. Setting REPLICA IDENTITY as suggested i don't think would fix the problem, and potentially introduces performance issues.

May i suggest that we merge the old and new record, such that missing columns don't get lost?

Something like:

-this.recordById.set(record.id, record);
+const mergedRecord = { ...this.recordById.get(record.id), ...record };
+this.recordById.set(record.id, mergedRecord);

For what it's worth i've forked your code and made this change and it's been working great on my end. Not sure if there are cases where it may break?

@aslakhellesoy
Copy link
Contributor

Good catch @ianschmitz! Please see #3 for a fix. In particular the sequence diagram generated from the new test.

If you're happy with this I'll merge and release.

@aslakhellesoy
Copy link
Contributor

Hi there 👋. Thanks for creating this lib. I'm surprised something like this isn't built into Supabase yet!

It would indeed be nice if this library was built into Supabase. OTOH, it might be a bit too specific to warrant an inclusion there.

But if someone from Supabase discovers it and decides to include it, I'd be pumped!

@aslakhellesoy
Copy link
Contributor

By the way @ianschmitz - would you be able to share what product you're using this library in? I'd love to list it under the in the wild section.

@aslakhellesoy
Copy link
Contributor

Fixed by #3 and released in 0.1.0

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

No branches or pull requests

2 participants