-
Notifications
You must be signed in to change notification settings - Fork 452
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
Do not invert
automatically in Doc.prototype._rollback
#442
Comments
Quick hacky fix is to just: delete types.json1.invert; But obviously that's pretty ugly, and you may or may not want to use In general, I'd expect if a type implements |
Is this a duplicate of #451? |
I did a more targeted and backwards-compatible fix in #520 - Fall back to doing a _hardRollback when op.type.invert fails. Let me know if you have any feedback on that! We're running that in a patched production build already |
Hi! I Just saw this change. |
Noted. I think I'm in favour of deprecating "soft" rollback as default behaviour. I'm not sure |
One more possible case: Say we have op1, op2, op3, op4, and all of op2 ~ op4 depend on the existence of op1 to be meaningful. Op1 is rejected by server and _rollback successful, which leaves op2 ~ op4 transformed against invert(op1). This results in transformed op2', op3', op4'. My question is: will op2', op3', op4' still be meaningful? Ideally, op2 ~ op4 should be removed along with op1. In the current framework of sharedb, the best bet may be to make the transform result of op2~op4 against invert(op1) to be no-op. |
Hey ShareDB's team!
While using https://github.com/ottypes/json1 with ShareDB, I found a small problem in
Doc.prototype._rollback
. It automatically usesJSON1.type.invert
but since we don't use invertible operations (our remove ops do not contain the removed data), it does not work.sharedb/lib/client/doc.js
Lines 950 to 957 in fa9179d
Moreover JSON1 support for invertible operations and composition is known to not be without flaws.
Do you think we could introduce a configuration option to deactivate this functionality?
The text was updated successfully, but these errors were encountered: