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

💥 Rename Doc op events #486

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

💥 Rename Doc op events #486

wants to merge 1 commit into from

Conversation

alecgibson
Copy link
Collaborator

@alecgibson alecgibson commented Jun 15, 2021

Fixes:

The Doc op events are currently a little confusing: a json0 "op" can
be shattered into multiple "component ops".

In the current naming, the "op" emits a 'op batch' event, and the
"op component" emits an 'op' event.

However, calling the op an "op batch" is a little bit misleading,
because the "batch" is always an op representing a single version number
increase, and potentially considered a single conceptual change.

For example, a remote client might submit a single op:

[
  {p: ['a'], oi: 'foo'},
  {p: ['b'], oi: 'bar'},
]

Under the current naming scheme, this emits the following events:

  1. 'before op batch': [{p: ['a'], oi: 'foo'}, {p: ['b'], oi: 'bar'}]
  2. 'before op': [{p: ['a'], oi: 'foo'}]
  3. 'op': [{p: ['a'], oi: 'foo'}]
  4. 'before op': [{p: ['b'], oi: 'bar'}]
  5. 'op': [{p: ['b'], oi: 'bar'}]
  6. 'op batch': [{p: ['a'], oi: 'foo'}, {p: ['b'], oi: 'bar'}]

This can be considered a little surprising, and you may expect the
'op' event to be emitted after the whole op (not its shattered
components) have been applied.

Under the new naming scheme, the following events are emitted:

  1. 'beforeOp': [{p: ['a'], oi: 'foo'}, {p: ['b'], oi: 'bar'}]
  2. 'beforeOpComponent': [{p: ['a'], oi: 'foo'}]
  3. 'opComponent': [{p: ['a'], oi: 'foo'}]
  4. 'beforeOpComponent': [{p: ['b'], oi: 'bar'}]
  5. 'opComponent': [{p: ['b'], oi: 'bar'}]
  6. 'op': [{p: ['a'], oi: 'foo'}, {p: ['b'], oi: 'bar'}]

This way, you get the 'op' event after the whole op has been applied.
It also makes it more explicit that you're actively listening out for
the shattered op components where applicable.

Note that we also move the events to camelCase to be consistent with
the Backend middleware actions.

@coveralls
Copy link

coveralls commented Jun 15, 2021

Coverage Status

Coverage remained the same at 97.398% when pulling df3679a on rename-op-events into f2a27fc on v2.

The `Doc` op events are currently a little confusing: a `json0` "op" can
be shattered into multiple "component ops".

In the current naming, the "op" emits a `'op batch'` event, and the
"op component" emits an `'op'` event.

However, calling the op an "op batch" is a little bit misleading,
because the "batch" is always an op representing a single version number
increase, and potentially considered a single conceptual change.

For example, a remote client might submit a single op:

```js
[
  {p: ['a'], oi: 'foo'},
  {p: ['b'], oi: 'bar'},
]
```

Under the **current** naming scheme, this emits the following events:

 1 `'before op batch'`: `[{p: ['a'], oi: 'foo'}, {p: ['b'], oi: 'bar'}]`
 2 `'before op'`: `[{p: ['a'], oi: 'foo'}]`
 3 `'op'`: `[{p: ['a'], oi: 'foo'}]`
 4 `'before op'`: `[{p: ['b'], oi: 'bar'}]`
 5 `'op'`: `[{p: ['b'], oi: 'bar'}]`
 6 `'op batch'`: `[{p: ['a'], oi: 'foo'}, {p: ['b'], oi: 'bar'}]`

This can be considered a little surprising, and you may expect the
`'op'` event to be emitted after the whole op (not its shattered
components) have been applied.

Under the **new** naming scheme, the following events are emitted:

 1 `'beforeOp'`: `[{p: ['a'], oi: 'foo'}, {p: ['b'], oi: 'bar'}]`
 2 `'beforeOpComponent'`: `[{p: ['a'], oi: 'foo'}]`
 3 `'opComponent'`: `[{p: ['a'], oi: 'foo'}]`
 4 `'beforeOpComponent'`: `[{p: ['b'], oi: 'bar'}]`
 5 `'opComponent'`: `[{p: ['b'], oi: 'bar'}]`
 6 `'op'`: `[{p: ['a'], oi: 'foo'}, {p: ['b'], oi: 'bar'}]`

 This way, you get the `'op'` event after the whole op has been applied.
 It also makes it more explicit that you're actively listening out for
 the shattered op components where applicable.

 Note that we also move the events to camelCase to be consistent with
 the Backend middleware actions.
@alecgibson
Copy link
Collaborator Author

As discussed with @ericyhwang :

  • camelCase is fine (consistent with our own Backend, and with Node.js)
  • Would be nice to not reuse the 'op' event, so that we can provide a migration pathway from v1
  • Could be nice to unwrap the single-item arrays that we currently pass to the "op component" event
  • Roll up 2nd arg into an object
  • Naming ideas:
    • opComponent & opComponentsList
    • change & subChange / update & partialUpdate
    • opApply & opComponentApply

@ericyhwang
Copy link
Contributor

Another idea from today:

  • [before]OpApply and [before]Json0Component[Apply]

@ericyhwang ericyhwang changed the base branch from v2 to master September 14, 2023 23:53
@ericyhwang ericyhwang changed the base branch from master to v2 September 14, 2023 23:57
@ericyhwang
Copy link
Contributor

(Was seeing if we could delete the v2 branch, which this PR points to, but that causes the diff to get wonky. Best leave it alone for now, probably.)

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.

3 participants