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

Adds two new doc events: 'before op batch' and 'after op batch' #247

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

danburzo
Copy link

We have recently upgraded from ShareJS to ShareDB following the migration guide. We've replaced after op with the op event, but we have found that op is called for each individual operation, as opposed to after op which had been called after an entire batch of operations.

The addition of two new events that get called at the start, and the end, of a batch of operations allows us perform the optimizations we were doing previously for when the client receives a batch of remote operations.

This pull request introduces these events:

  • before op batch is called before any before op event in a batch
  • after op batch is called after any op event in a batch

They currently receive no parameters, but we can change that if any parameters would be appropriate here.

Best regards!

@coveralls
Copy link

coveralls commented Sep 13, 2018

Coverage Status

Coverage increased (+0.006%) to 95.855% when pulling 53b9109 on Evercoder:master into 7938f7f on share:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.007%) to 95.517% when pulling 60509ba on Evercoder:master into e45010c on share:master.

@danburzo
Copy link
Author

danburzo commented Sep 24, 2018

Is there any chance we can get this merged soon? It's causing some issues for us that are hard to tackle without these events in place.

Thank you!

@danburzo
Copy link
Author

We ended up publishing our fork in the interim.

@adelriosantiago
Copy link

I was looking exactly for this. the op event triggers several times individually. And each change is actually broadcast'ed to all users. Will this PR land master?

@alecgibson
Copy link
Collaborator

I'm not entirely sure I understand the motivation behind this change. What's the use case?

@danburzo
Copy link
Author

danburzo commented Jun 9, 2020

When receiving a batch of events, it can be beneficial to be able to tell when the batch starts and ends, so that you may pause side-effects at the beginning of the batch, and only update and resume when you've finished incorporating all the (potentially hundreds of) individual ops.

In a previous version of sharedb, after op would trigger after the entire operation batch is finished, and following the suggestion from the upgrade guide to switch from after op to op incurred a significant performance penalty.

@alecgibson
Copy link
Collaborator

@danburzo sounds fairly reasonable to me. I'll see if I can discuss with other contributors tomorrow. My one reservation about it is that it would be an event that only ever has relevance to the default type, but json0 is something of a special case across all of sharedb, so I've certainly heard weirder things.

@alecgibson
Copy link
Collaborator

Had a discussion with @ericyhwang . In general, this should be fine. It is a bit weird that it'll only affect the default type, but this doesn't preclude us from extending to other types later, and JSON0 already gets slightly special treatment (eg projections).

Could you please:

  • Rebase
  • Change event names to: beforeMultiComponentOp and afterMultiComponentOp (we'll separately camel-case before op and after op — we somehow missed this in the v1 release)
  • Only emit the events for these shattered JSON0 ops — you should just be able to remove the length check here, and then remove the new events you've outside that if block

@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.

5 participants