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

Do not compose create with op #213

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gkubisa
Copy link
Contributor

@gkubisa gkubisa commented Jun 12, 2018

The problem is that the create operation contains initial data rather than a snapshot and apply requires the first param to be a snapshot. So, the original implementation worked only for the types which have the same type of initial data and snapshots.

The problem is that the `create` operation contains initial data
rather than a snapshot and `apply` requires the first param to be
a snapshot. So, the original implementation worked only for the
types which have the same type of initial data and snapshots.
@coveralls
Copy link

coveralls commented Jun 12, 2018

Coverage Status

Coverage decreased (-0.005%) to 96.487% when pulling d1ca2ae on Teamwork:fix-tryCompose into 68bde00 on share:master.

@ericyhwang
Copy link
Contributor

From the PR review meeting, @gkubisa linked #214 as an example of where this can cause an issue, with the custom text-tp2 OT type.

@nateps's comments:

  • Problem with compose is that you lose information about the context [of the original ops].
  • Also, it's possible for one of the ops to fail.
  • Safest extreme is to compose nothing, but composing is good in many situations like typing text, or where you might want things to go together to the server.
  • Possible for something to rely on compose, that would be broken by not composing.
  • It's an interesting question as to whether compose should operate on creates. The spec doesn't allow it, but it is a performance benefit.

Greg suggests, if we do want to keep the performance benefit, adding an applySerialized function to OT types, so that we can use it if it's available and not break the spec.

@nateps says that after talking about it, let's just merge this.

@gkubisa
Copy link
Contributor Author

gkubisa commented Jul 12, 2018

Just to clarify, I agree with @nateps that the performance impact of this change is negligible.

I mentioned applySerialized and its potentially significant impact on performance in the context of updating ShareDB to correctly support the OT Types spec. See #214 (comment) for more details.

@curran
Copy link
Contributor

curran commented Jul 22, 2020

Suggest to close as stale.

@curran curran mentioned this pull request Oct 21, 2020
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

Successfully merging this pull request may close these issues.

4 participants