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

receive op data had split more times with not local? #396

Closed
aeroyu opened this issue Oct 13, 2020 · 7 comments
Closed

receive op data had split more times with not local? #396

aeroyu opened this issue Oct 13, 2020 · 7 comments

Comments

@aeroyu
Copy link

aeroyu commented Oct 13, 2020

i submit op like
[
{
"sd": "checkable-list-item",
"p": [
"blocks",
0,
"type",
0
]
},
{
"si": "unstyled",
"p": [
"blocks",
0,
"type",
0
]
}
]
and local receive op event op is the same with send data,
but with other client receive op,has split two time,[
{
"sd": "checkable-list-item",
"p": [
"blocks",
0,
"type",
0
]
}] and [{
"si": "unstyled",
"p": [
"blocks",
0,
"type",
0
]
}
]

how can i receive op normal

@alecgibson
Copy link
Collaborator

Could you please elaborate on your use case? Why is it important to have these delivered as a single object?

@aeroyu
Copy link
Author

aeroyu commented Oct 13, 2020

Could you please elaborate on your use case? Why is it important to have these delivered as a single object?

i use sharedb with draft js,when the op is split and setState,will not work,ex [ { "sd": "unstyled", "p": [ "blocks", 0, "type", 0 ] }, { "si": "checkable-list-item", "p": [ "blocks", 0, "type", 0 ] } ]
the first op (delete type unstyled) merge into local ,but it is not my expect,because ,draft js block type can not be empty,and will reset type to unstyled,after apply the second op , the type result is “checkable-list-itemunstyled”

@aeroyu
Copy link
Author

aeroyu commented Oct 13, 2020

Could you please elaborate on your use case? Why is it important to have these delivered as a single object?

i have solve it,i solution is set the uuid and op_size to op model,when receive by other side,i get uuid and op_size to merge the ops and do one setState,it work!any best solution?

@alecgibson
Copy link
Collaborator

Glad to hear you solved it. Honestly off the top of my head I'm not sure there's any (easy) way around it. If you're interested, here's the code that's responsible for that behaviour:

sharedb/lib/client/doc.js

Lines 584 to 620 in c711cfc

// Iteratively apply multi-component remote operations and rollback ops
// (source === false) for the default JSON0 OT type. It could use
// type.shatter(), but since this code is so specific to use cases for the
// JSON0 type and ShareDB explicitly bundles the default type, we might as
// well write it this way and save needing to iterate through the op
// components twice.
//
// Ideally, we would not need this extra complexity. However, it is
// helpful for implementing bindings that update DOM nodes and other
// stateful objects by translating op events directly into corresponding
// mutations. Such bindings are most easily written as responding to
// individual op components one at a time in order, and it is important
// that the snapshot only include updates from the particular op component
// at the time of emission. Eliminating this would require rethinking how
// such external bindings are implemented.
if (!source && this.type === types.defaultType && op.op.length > 1) {
if (!this.applyStack) this.applyStack = [];
var stackLength = this.applyStack.length;
for (var i = 0; i < op.op.length; i++) {
var component = op.op[i];
var componentOp = {op: [component]};
// Transform componentOp against any ops that have been submitted
// sychronously inside of an op event handler since we began apply of
// our operation
for (var j = stackLength; j < this.applyStack.length; j++) {
var transformErr = transformX(this.applyStack[j], componentOp);
if (transformErr) return this._hardRollback(transformErr);
}
// Apply the individual op component
this.emit('before op', componentOp.op, source, op.src);
this.data = this.type.apply(this.data, componentOp.op);
this.emit('op', componentOp.op, source, op.src);
}
// Pop whatever was submitted since we started applying this op
this._popApplyStack(stackLength);
return;
}

You'll see it performs this check:

if (!source && this.type === types.defaultType && op.op.length > 1)

and then "shatters" the op.

So one potential way around this would be to register second json0 type, which isn't the default type, and use that, but it's pretty hacky.

You could also try switching to the json1 type, which seems to have more semantic operations (including replace).

The final thing — which I think is what you've landed on? — is to buffer incoming ops and batch them together.

Of course you could always raise a pull request to disable this behaviour using some sort of config flag!

@aeroyu
Copy link
Author

aeroyu commented Oct 24, 2020

Glad to hear you solved it. Honestly off the top of my head I'm not sure there's any (easy) way around it. If you're interested, here's the code that's responsible for that behaviour:

sharedb/lib/client/doc.js

Lines 584 to 620 in c711cfc

// Iteratively apply multi-component remote operations and rollback ops
// (source === false) for the default JSON0 OT type. It could use
// type.shatter(), but since this code is so specific to use cases for the
// JSON0 type and ShareDB explicitly bundles the default type, we might as
// well write it this way and save needing to iterate through the op
// components twice.
//
// Ideally, we would not need this extra complexity. However, it is
// helpful for implementing bindings that update DOM nodes and other
// stateful objects by translating op events directly into corresponding
// mutations. Such bindings are most easily written as responding to
// individual op components one at a time in order, and it is important
// that the snapshot only include updates from the particular op component
// at the time of emission. Eliminating this would require rethinking how
// such external bindings are implemented.
if (!source && this.type === types.defaultType && op.op.length > 1) {
if (!this.applyStack) this.applyStack = [];
var stackLength = this.applyStack.length;
for (var i = 0; i < op.op.length; i++) {
var component = op.op[i];
var componentOp = {op: [component]};
// Transform componentOp against any ops that have been submitted
// sychronously inside of an op event handler since we began apply of
// our operation
for (var j = stackLength; j < this.applyStack.length; j++) {
var transformErr = transformX(this.applyStack[j], componentOp);
if (transformErr) return this._hardRollback(transformErr);
}
// Apply the individual op component
this.emit('before op', componentOp.op, source, op.src);
this.data = this.type.apply(this.data, componentOp.op);
this.emit('op', componentOp.op, source, op.src);
}
// Pop whatever was submitted since we started applying this op
this._popApplyStack(stackLength);
return;
}

You'll see it performs this check:

if (!source && this.type === types.defaultType && op.op.length > 1)

and then "shatters" the op.

So one potential way around this would be to register second json0 type, which isn't the default type, and use that, but it's pretty hacky.

You could also try switching to the json1 type, which seems to have more semantic operations (including replace).

The final thing — which I think is what you've landed on? — is to buffer incoming ops and batch them together.

Of course you could always raise a pull request to disable this behaviour using some sort of config flag!

thx

alecgibson pushed a commit that referenced this issue Nov 4, 2020
This change adds two events:

 - `before op batch` - fired once before any set of ops is applied.
 - `op batch` - fired once after any set of ops is applied. This may
   follow multiple `op` events if the op had multiple `json0` components

This is a non-breaking change that should allow clients to process ops
in their entirety.

There has already been some discussion around this:

 - #129
 - #396

This is a much simpler approach than the existing pull request. Here we
try not to change existing behaviour, and only add new, non-breaking
events.

Motivation for such an event would include clients applying some form of
validation logic, which doesn't make sense if an op is shattered.

For example, consider a client that wants to ensure a field
`mustBePresent` is always populated:

```js
doc.on('op', () => {
  if (!doc.data.mustBePresent) throw new Error('invalid');
});

remoteDoc.submitOp([
  {p: ['mustBePresent', 0], sd: 'existing value'},
  {p: ['mustBePresent', 0], si: 'new value'},
]);
```

In the above example, the submitted op is clearly attempting to perform
a replacement. However, the receiving `doc` only receives this
replacement in parts, so it looks like the document reaches an invalid
state, when actually the submitted op is perfectly valid.

In this case we `throw`, but we could have also attempted to populate
with a default value, which could interfere with the desired value.

This change fixes the above issue, because now we can just listen for
the `op batch` event, and consider the document once all the components
of a given op have been applied.
alecgibson pushed a commit that referenced this issue Nov 11, 2020
This change adds two events:

 - `before op batch` - fired once before any set of ops is applied.
 - `op batch` - fired once after any set of ops is applied. This may
   follow multiple `op` events if the op had multiple `json0` components

This is a non-breaking change that should allow clients to process ops
in their entirety.

There has already been some discussion around this:

 - #129
 - #396

This is a much simpler approach than the existing pull request. Here we
try not to change existing behaviour, and only add new, non-breaking
events.

Motivation for such an event would include clients applying some form of
validation logic, which doesn't make sense if an op is shattered.

For example, consider a client that wants to ensure a field
`mustBePresent` is always populated:

```js
doc.on('op', () => {
  if (!doc.data.mustBePresent) throw new Error('invalid');
});

remoteDoc.submitOp([
  {p: ['mustBePresent', 0], sd: 'existing value'},
  {p: ['mustBePresent', 0], si: 'new value'},
]);
```

In the above example, the submitted op is clearly attempting to perform
a replacement. However, the receiving `doc` only receives this
replacement in parts, so it looks like the document reaches an invalid
state, when actually the submitted op is perfectly valid.

In this case we `throw`, but we could have also attempted to populate
with a default value, which could interfere with the desired value.

This change fixes the above issue, because now we can just listen for
the `op batch` event, and consider the document once all the components
of a given op have been applied.
@alecgibson
Copy link
Collaborator

@aeroyu this should now be fixed in v1.5.0 — you can listen for the op batch event instead of op

@aeroyu
Copy link
Author

aeroyu commented Dec 1, 2020

thx , great help to me

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