feat: trigger reactive queries for lists on executeBatch#299
Conversation
| } | ||
|
|
||
|
|
||
| std::optional<std::string> extract_modified_table(const std::string &sql) { |
There was a problem hiding this comment.
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.
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.
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.
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.
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.
|
Took a quick look at this. There is no need to modify the native code. Execute batch should already trigger hooks and all that was needed was to call flushPendingReactiveQueries from the JS code of executeBatch. Closing this in favor of #318 |
2 participants
fixes #296