-
-
Notifications
You must be signed in to change notification settings - Fork 65
feat: trigger reactive queries for lists on executeBatch #299
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
base: main
Are you sure you want to change the base?
Conversation
@@ -324,4 +325,44 @@ void log_to_console(jsi::Runtime &runtime, const std::string &message) { | |||
log.call(runtime, jsi::String::createFromUtf8(runtime, message)); | |||
} | |||
|
|||
|
|||
std::optional<std::string> extract_modified_table(const std::string &sql) { |
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.
Yeah... I'm sorry, but I will probably not merge this. This is brittle and maintaining when people complain it's not working for their very particular use-case is going to bring me too much work, which I don't feel like doing. Sorry :) but feel free to fork your own version, it's MIT
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.
Ok I could live with a patch, but if you have a better idea to implement this, I would be happy to update my pr, maybe we could add an api to manually trigger update on js side ?
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.
No thoughts from the top of my head, sorry, it's been a while since I implemented reactive queries. But basically, how it's working on transactions is the only way that is generic enough and not crazy. I believe they flush the queued updates from JS already but also, I don't want to expose the internals functions again because people complain too much and don't read the internal C++ so they end up dumping work on my lap.
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 understand that you have to limit the api surface for maintenance purpose, I didn't see flushPendingReactiveQueries
earlier, but without a way to update pending_reactive_queries
, I can’t use it. Maybe we could find a mechanism on reactiveExecute
itself, with a specific fireOn
flag to react to external events
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.
pending_reactive_queries should be updated on their on based on the update hook, they don't need manual updating as far as I remember, but again, I already forgot what's in here. You need to read the whole C++ code to understand how things are connected.
fixes #296