Add PersistedModel.subscribe() #1445
Conversation
| } | ||
|
|
||
| var where = ctx.where; | ||
| var data = ctx.instance || ctx.data; |
There was a problem hiding this comment.
ctx.instance.toJSON() || ctx.data?
|
As mentioned in the slack chat, I can't review this thoroughly right now. I did a quick review and didn't find any major issues, I think it's ok to merge this as it is and refine the implementation later as needed. |
|
This method is being renamed, correct? |
| }); | ||
|
|
||
| Model.observe('before save', createChangeHandler('save', true)); | ||
| Model.observe('before delete', createChangeHandler('delete', true)); |
There was a problem hiding this comment.
In my opinion it does not make much sense to send optimistic changes from "before" hooks.
- When running on the server, the latency of database calls is usually negligible, thus an "optimistic" update does provide much value IMO.
- It would make sense to emit optimistic changes when running in the browser, but IIRC, the isomorphic client does not invoke operation hooks because the remote connector completely bypasses DAO methods.
I think we need to find a different solution for emitting optimistic updates.
There was a problem hiding this comment.
When running on the server, the latency of database calls is usually negligible, thus an "optimistic" update does provide much value IMO.
The purpose of an optimistic update is to avoid latency entirely. We don't have to wait for the operation to be acknowledged by the database (or rest api, other datasource) since we are only telling the client about a likely change.
I think we are disagreeing on what latency is acceptable. IMO any IO is going to add significant latency. Ideally we would emit the optimistic change right after it is sanitized. Ideally it would be emitted before timely ACL checks, but this introduces obvious vulnerabilities.
In order to move this forward, I am OK with removing this code, as it can be added later as an optimization.
|
|
||
| changes.destroy = function() { | ||
| changes.removeAllListeners('error'); | ||
| changes.removeAllListeners('end'); |
There was a problem hiding this comment.
It would be good to remove hook observers too.
I think that otherwise the list of registered hook observers will grow without limits, each DAO operation will have to invoke a lot of inactive hooks and the server will gradually slow down.
@BerkeleyTrue Yep... renamed to |
|
@bajtos I'm seeing some errors in the phantomjs tests that appear to be unrelated to this change. I'm getting them on master as well. I'd like to fix those test in another PR. Any objections? |
| @@ -81,7 +81,7 @@ | |||
| "karma-script-launcher": "^0.1.0", | |||
| "loopback-boot": "^2.7.0", | |||
| "loopback-datasource-juggler": "^2.19.1", | |||
| "loopback-testing": "^1.1.0", | |||
| "loopback-testing": "~1.1.0", | |||
There was a problem hiding this comment.
Is there any particular reason for locking down loopback-testing version to 1.1.x?
Sure, that's fine with me. |
|
Some of my older comments were not addressed, I re-posted them on the updated diff. None of them is a blocker though, feel free to land this PR without addressing them. For the sake of our future selves, it would be good to explain why do you need to lock the version of loopback-testing to |
1.2 introduces a bug. strongloop-archive/loopback-testing#62 |
|
test please |
Connect to #1444