Inserting non text characters into Automerge.Text

[Gozala]

This is related to #193 as I attempt to model rich text as series of tokens of characters and markers:

{format:{italic:true}}Hello {format:{bold:true, italic:true}}beautiful{format:null} world\n

As I was trying to figure out a best approach to this I noticed that Automerge.Text would happily insert objects e.g. text.insertAt(0, { bold: true }) which than would appear as objectIds inside the text. (related thread on slack) It's also worth mentioning that Automerge.getObjectById(doc, objectId) would actually return inserted object.

I'm not sure what is the intended behavior here, but I suspect that text.join('') including objectId is highly unlikely. It seemed like a convenient accident for my use case, but I'm suspicious there might be deeper consequences so I chose to use list (or should I say array) of tokens instead. Either way I would like to propose:

1. To document how Automerge.Text outperforms arrays (as suggested by the readme).
2. Either throw if non-text is being inserted or omit objectIds in .join() and document non textual inserts bit more.

[ept]

I think it would be good for Automerge.Text to support marker objects, and the current behaviour is broken. We should definitely change it so that text.insertAt(0, {bold: true}) works, and when you follow it with text.get(0) you get back the {bold: true} object.

Not sure what .join() should do in this case. Maybe simply skip over any marker objects?

[Gozala]

I think it would be good for Automerge.Text to support marker objects, and the current behaviour is broken. We should definitely change it so that text.insertAt(0, {bold: true}) works, and when you follow it with text.get(0) you get back the {bold: true} object.

Not sure what .join() should do in this case. Maybe simply skip over any marker objects?

That is exactly what my RichText class that wraps list of tokens does. I'd love to let Automerge.Text do that.

[Gozala]

Actually currently my RichText doesn't take just {bold:true} instead it looks more like this:

export type Resource = {[string]:string|number|boolean|null} 
export type Attributes = {[string]:string|number|boolean|null}
export type CharacterToken = string
export type MarkerToken = {|attributes:Attributes, length:0|}
export type EmbedToken = {|resource:Resource, attributes:?Attributes, length:1|}

export type Token =
  | CharacterToken
  | EmbedToken
  | MarkerToken

export type RichTextBuffer = MutableList<Token>

I think it might be more reasonable to add char: ?string field to MarkerToken and EmbedToken and let .join() use it if present or omit otherwise.

[Gozala]

We should definitely change it so that text.insertAt(0, {bold: true}) works, and when you follow it with text.get(0) you get back the {bold: true} object.

I've tried to write a patch for this, but I got stuck - As far as I can tell Text instances only hold elems that don't contain any references to inserted objects nor nor to a document to be able to get corresponding objects from.

I suspect this is where change needs to occur but I'm not confident make those chanages
https://github.com/automerge/automerge/blob/d05b2fee907668ff10512fa2715e25e58e23ce2a/frontend/apply_patch.js#L344

[Gozala]

Not sure what .join() should do in this case. Maybe simply skip over any marker objects?

@ept Is .join() on Text is even necessary, I realize it end up there because Text is list of characters, however conceptually it more like a JS String which does not have a .join method. I also don't really find it all that useful for Text.

[Gozala]

think it would be good for Automerge.Text to support marker objects, and the current behaviour is broken. We should definitely change it so that text.insertAt(0, {bold: true}) works, and when you follow it with text.get(0) you get back the {bold: true} object.

I was thinking more about this & I'm starting to think that it would be better to have two distinct types Automerge.Text for plaint text (and no marker token) and a separate (maybe Automerge.Markup) for text and markers. That would reduce margin for error (I have discovered that non text was supported by mistake text.insertAt(offset, ["h", "e", "l", "l", "o"]) did not realize I had to pass chars as args). And more appropriate API could be chosen for text with markers.

[mattkrick]

not sure if my mention was a mistake, but here's my .02

IIRC Text is just shorthand for an array of chars. if the text is rich, just use a doc.
i ran out of dev time for it, but an example is here: https://github.com/mattkrick/rich

see atlaskit by atlassian and prosemirror, which it's built on. if you wanted to build a CRDT plugin on top of prosemirror, it'd be revolutionary, although last time i checked it was too hard-wired for OTs.

as for type errors, i'd recommend just PRing the types for this package & using typescript.

[Gozala]

not sure if my mention was a mistake, but here's my .02

My apologies I was trying to mention @ept by his name & have not noticed that github autocompleted with your handle instead.

[Gozala]

IIRC Text is just shorthand for an array of chars. if the text is rich, just use a doc.
i ran out of dev time for it, but an example is here: https://github.com/mattkrick/rich

I don't think it's that simple as there are many cases where intention preservation will suffer, this is discussed in greater detail at #193

see atlaskit by atlassian and prosemirror, which it's built on. if you wanted to build a CRDT plugin on top of prosemirror, it'd be revolutionary, although last time i checked it was too hard-wired for OTs.

I intent to get to prosemirror eventually, but I chose to go with quill in first iteration as it's data model maps CRDTs much better. Problem with ProseMirror is that data model invariants are encoded into a document schema & with CRDTs you can converge to state that invalidates those invariants and there for ProseMirror will reject. This implies there needs to be a mechanism for incorporating changes from other replicas in a way that would comply with a schema.

as for type errors, i'd recommend just PRing the types for this package & using typescript.

My comment was less about type errors and more about type signatures. Currently it's insertAt(number, ...Array<object|string>) which means following type-checks text.insertAt(offset, ["h", "e", "l", "l", "o"]) even though it's not what was intended. That is also why I think having separate data types is good idea.

[ept]

The original issue discussed here (support for objects inside Automerge.Text) is fixed in #238. The other problems around the type signature of the Text class can be dealt with in a separate issue.