Make Annotations classes instead of JS objects #54
Conversation
This simple source provides a look into how we're changing how annotations are created from JSON
This moves the logic for altering annotations into the Annotation class and encodes vendor prefixing into annotations by default
|
|
||
| export default abstract class BlockAnnotation extends Annotation { | ||
| rank() { | ||
| return 1000; |
There was a problem hiding this comment.
Thanks for explaining rank irl, and as you pointed out rank numbers should match :) https://github.com/CondeNast-Copilot/atjson/blob/ae08b75444fd091da016409edb0a9f871726f0a4/packages/@atjson/hir/src/hir-node.ts#L5-L13
| if (key.indexOf(`-${vendorPrefix}-`) === 0) { | ||
| copy[key.slice(`-${vendorPrefix}-`.length)] = unprefixAttribute(vendorPrefix, value, annotation) as AttributeValue | AttributeValue[] | undefined; | ||
| } | ||
| return copy; |
There was a problem hiding this comment.
Q: could this case just call the unprefixAttributes function defined below?
|
|
||
| } else { | ||
|
|
||
| if (change.end < this.end) { |
There was a problem hiding this comment.
trivial - could this be combined as else if to reduce conditional nesting?
(just to make things a tad easier to follow)
also maybe I'm reading it wrong but I think you could.
| // n.b. the += 0 is just to silence tslint ;-) | ||
| } else if (change.start === this.end) { | ||
| this.end += 0; | ||
| } |
There was a problem hiding this comment.
silencing tslint because it would have complained about an empty block? but do you need this block at all? or is it here just to serve as living documentation? (an else case would also do nothing)
There was a problem hiding this comment.
Living documentation here.
| this.content = this.content.slice(0, start) + text + this.content.slice(start); | ||
| } catch (e) { | ||
| // tslint:disable-next-line:no-console | ||
| console.error('Failed to insert text', e); |
There was a problem hiding this comment.
Q: why console instead of throw?
There was a problem hiding this comment.
Probably should throw!
There was a problem hiding this comment.
Hmm. I saw the context for this again. I think we're doing this to guard from non-atjson code from causing apps to blow up. We will revisit this, for sure.
There was a problem hiding this comment.
Makes sense. As a possible follow-up for another PR, maybe own your own logger interface. Then you can control if/when to output and to where.
|
@pgoldrbx ❤️ Thanks for the review ❤️ |
There was a problem hiding this comment.
This is great!
One question: This has a breaking change on at least deleteText, presumably [add|remove]Annotations() work the same. Is it worth adding backward-compatibility with deleteText and adding a deprecation warning so that we can find the problem spots, or am I over-complicating it? :)
| @@ -1,18 +1,142 @@ | |||
| import { Display } from './schema'; | |||
| import { Attributes, toJSON, unprefix } from './attributes'; | |||
| import Change, { AdjacentBoundaryBehaviour, Deletion, Insertion } from './change'; | |||
| abstract readonly type: string; | ||
| } | ||
|
|
||
| export enum AdjacentBoundaryBehaviour { |
| import Annotation from './annotation'; | ||
|
|
||
| export default abstract class InlineAnnotation extends Annotation { | ||
| rank() { |
There was a problem hiding this comment.
Worth making these getters?
There was a problem hiding this comment.
Yes! That sounds good!!
|
Not sure if it's worth making it backwards compatible rn, things will be obvious when they break |
This work makes it possible for us to richly extend our annotation model, enabling us to write a rich text editor (see the
editorpackage). Specifically, annotations likeparagraph,blockquote, andlistrequire special behavior when newlines are inserted into the document.To do this in a flexible / extensible way, we need to encode this behavior into classes instead of colocating the logic into the root document.
The highlight of changes is:
1️⃣ The
schemais a list of annotations to use to hydrate JSON passed into the document.2️⃣ Annotations at rest now require vendor prefixes to by hydrated correctly. This is done to ensure that we do not have collisions between annotations with the same name. (cf. a Markdown
blockquoteto a HTMLblockquote)3️⃣
attributesare required. Although annoying in tests, it removes a lot of defensive code that would otherwise need to be written4️⃣ Annotations undergo a structured clone when being passed to the query language, which means that altering annotations will no longer have side-effects on the document
5️⃣ The
Changeobject, along with theInsertionandDeletionobjects are designed to provide aninitial abstraction layer for us to start organizing document changes in a way that allows for a real-time system to be written on top of AtJSON. Over time, this will become simpler as more common patterns are adapted into AtJSON.
6️⃣
ParseAnnotationis now a concrete annotation, and is a special type of annotation that is always included in the schema list. These annotations are used internally byAtJSONto remove underlying text nodes. (This annotation may be renamed in the future to be more friendly for this use-case).