Support complex / deep / nested attributes #48
Conversation
This change adds support for complex attributes, which might have
arbitrarily nested properties:
```json
{
"insert": "some text",
"attributes": {
"complexAttribute": {
"prop1": {
"subPropA": "a",
"subPropB": 2
},
"prop2": true
}
}
}
```
## Motivation
The particular use case with which this was written in mind is the
addition of comments to Quill. That is, the ability to select part of a
document, and add some extra information about that selection.
Comments can be applied to arbitrary content, so cannot be represented
as an embed (for example). They naturally belong to the attributes:
```json
{
"insert": "This text is commented",
"attributes": {
"comment": "comment-id"
}
}
```
The issue with comments is that multiple comments may exist on the same
range:
```json
{
"insert": "this text is commented",
"attributes": {
"comment": {
"comment-id-1": true,
"comment-id-2": true
}
}
}
```
These cannot be "flattened", because the IDs are arbitrary strings,
which:
- could collide with other attributes who rely on IDs like this
- does not map neatly to a Quill attributor / blot
Any other metadata-like attributes may also benefit from this sort of
deeply nested structure. For example, a naive git-blame-like feature
might look like:
```json
{
"insert": "\n",
"attributes": {
"blame": {
"hash1": {
"author": "Alec Gibson",
"timestamp": 1582123032,
}
}
}
}
```
## Implementation
This change adds support to all of `compose`, `diff`, `invert` and
`transform` for complex attributes.
The implementation is recursive, and each depth of attributes is
treated similarly to the top level. In particular:
- `null` will remove a property
- an empty object is treated as `null`
```javascript
const attributes = {complex: {foo: 123}}
const update = {complex: {foo: null}}
AttributeMap.compose(attributes, update) // => undefined
```
### Arrays
Note that arrays are out-of-scope of this change. Their behaviour is not
well defined when composing deltas together, and they are treated as if
they are primitive values (ie they directly overwrite one another,
rather than attempt a deep change).
## Quill integration
Quill will not natively support these changes, and further work will be
required if this capability is to be adopted there. Given that Quill v2
is currently in development, this may be a good time to do this work, if
desirable.
In particular:
- `format()` methods need to `compose` values rather than simply
overwrite
- `merge()` methods need to perform deep equality checks, rather than
object reference checks
Note that Quill can currently support these complex attributes, but only
with blots that override the above methods.
## Backwards compatibility
This change is not technically backwards-compatible. If anyone is
already using deeply nested attributes and relying on the current
all-or-nothing behaviour, then this change will break their code.
|
On backwards compatibility: I'm happy to hide this behind some sort of package-level flag (I just didn't do it before getting an opinion on how best to handle backwards compatibility) eg Delta.complexAttributes = true;And I'm happy to do whatever work is necessary in Quill to get this supported there, too. |
|
Great job! Need this feature very much. |
This change moves our `quill-delta` dependency onto our own forked version, which has support for [complex attributes][1], which we need for track changes. [1]: slab/delta#48
This change moves our `quill-delta` dependency onto our own forked version, which has support for [complex attributes][1], which we need for track changes. [1]: slab/delta#48
This change moves our `quill-delta` dependency onto our own forked version, which has support for [complex attributes][1], which we need for track changes. [1]: slab/delta#48
This change moves our `quill-delta` dependency onto our own forked version, which has support for [complex attributes][1], which we need for track changes. [1]: slab/delta#48
This change updates the dependency on `quill-delta` to point to our forked version, which adds support for [complex attributes][1] [1]: slab/delta#48
This change updates the dependency on `quill-delta` to point to our forked version, which adds support for [complex attributes][1] [1]: slab/delta#48
|
maybe the nested attributes is the correct way to implement tables |
|
I appreciate the effort here, especially the numerous tests but spec changes should be discussed with alternative solutions considered and weighed. The issues with this proposals at the moment:
|
|
@jhchen thanks for getting back to me. I'm more than happy to discuss spec changes — this PR was always designed to just get the conversation started — but where's the best forum for this? It's taken over a year to get a response on this PR, so I'm eager to move the conversation to wherever is best to actually get some back-and-forth going. Would you prefer I open an issue? ShareDB has weekly PR meetings; is there anything similar for Quill?
Apologies if I was unclear on the merge behaviour, I thought I explained in the description. Each "level" of properties is merged in the same way that the top level is. This should also be described by the tests.
This doesn't handle collisions, because in the case of simultaneous edits you'll just get last-write-wins: const comment1 = new Delta().retain(3, {comment: ['1']})
const comment2 = new Delta().retain(1).retain(3, {comment: ['2']})
const composed = comment1.compose(comment2)The above results in: [
{retain: 1, attributes: {comment: ['1']}},
{retain: 3, attributes: {comment: ['2']}},
]Which is no better than using a "raw" ID instead of an array. Please let me know if I'm missing something; as I say, I'm super happy to have a discussion about this wherever's best. |
|
Having a feature like this would be great. I'm trying to add some spell-checking functionality, and the attributes needed is more than just a value, as I need to render multiple things based on that. Flattening those attributes, as it happens now, makes it inconsistent: the first time a blot gets created the whole attribute gets created, but next time the document is modified, it's no longer the same type ComplexAttribute = {
key: string,
value: string
}
// somewhere in code
const complexAttribute = { key: "some key", value: "some value" }
const ops = new Delta()
.retain(10)
.retain(10, { complex: complexAttribute });
editor.updateContents(ops); // this one works perfectly
const ops2 = new Delta().insert('some more text')
editor.updateContents(ops2); // this one blows up on the complex Blot: attr is now true instead of a structure
// My Blot
class ComplexBlot extends ParentBlot {
static blotName = "complex";
static tagName = "quill-complex";
static create(attr?: ComplexAttribute) {
let node: HTMLElement = super.create();
if (attr) {
// first time
node.setAttribute("data-complex-key", attr.key);
node.setAttribute("data-complex-value", attr.value);
}
console.log("Created blot", node);
return node;
}
optimize() {}
};I would really appreciate if this was reconsidered... @alecgibson is making an amazing job, but having to keep forks of both |
|
For what it's worth, we've been running this in a Production environment on our own fork for a few years now, and it seems to be working pretty well. |
|
@alecgibson do you have plans to publish the packages for your fork? I'm happy to collaborate maintaining it if needed in case you decide to publish it. What I would hate is to have to do a fork of a fork 😂 |
|
@xavivars it's already published on GitHub packages, although it's admittedly not as smooth to use as on If you do start using our fork, it has got some other changes in there, so I'd check that it's suitable for your use. I don't think we've done anything else seriously major apart from the Complex Attributes, though. On the other hand, forks of forks aren't the craziest thing I've seen 🤷🏼 If the Complex Attributes stuff is all you're after, it's been pretty stable. The last change I made to it was back in 2020, and we're extremely unlikely to make any breaking changes to the logic, since we have Production code and database data that relies on it. |
|
I alredy reviewed the other changes, and I think all of them make sense, as they are mostly fixes. The one I'm not sure about is Parchment's fork, as I haven't really seen any major change (unless you've been rebasing and all your changes have been later pulled upstream). I also tried to install, changing .npmrc, but it seems the packages are private and can't be downloaded. Maybe that's a configuration at your end.
|
|
We actually moved back to the upstream I'll see if I can find time to publish to {
"dependencies": {
"quill": "reedsy/quill#2.0.0-reedsy-2.0.3"
}
} |
|
Oh, didn't know you could just point to a url! However, that doesn't work for the same reason, it tries to download quill-delta from github. BTW, no need to publish to npm, just changing visibility of the package should be enough |
|
@luin is there any chance of reopening this discussion for Quill v2 (or even v3)? |
|
@alecgibson Glad to see the idea works for you in prod! I did implement something similar in my previous company and my major concern of implementing this in Delta package is backward compatibility and the added complexity. Feel free to create a discussion on Quill repo to continue discussing the idea. I'll add a TODO note myself for this. btw: didn't find your email but would like to learn more about your use cases/requirements for Quill/Delta/Parchment and how we can improve to make things easier for you. Feel free to reach out at zihua@slab.com if you happen to have any thoughts! |
|
@luin I've opened a discussion here, just copying the Pull Request description: slab/quill#4000 This Pull Request wasn't necessarily meant to be the final implementation; it was meant to be a starting point for discussion, and I'm happy to do whatever work is necessary to get this merged if people think it can live in |
This change adds support for complex attributes, which might have
arbitrarily nested properties:
{ "insert": "some text", "attributes": { "complexAttribute": { "prop1": { "subPropA": "a", "subPropB": 2 }, "prop2": true } } }Motivation
The particular use case with which this was written in mind is the
addition of comments to Quill. That is, the ability to select part of a
document, and add some extra information about that selection.
Comments can be applied to arbitrary content, so cannot be represented
as an embed (for example). They naturally belong to the attributes:
{ "insert": "This text is commented", "attributes": { "comment": "comment-id" } }The issue with comments is that multiple comments may exist on the same
range:
{ "insert": "this text is commented", "attributes": { "comment": { "comment-id-1": true, "comment-id-2": true } } }These cannot be "flattened", because the IDs are arbitrary strings,
which:
Any other metadata-like attributes may also benefit from this sort of
deeply nested structure. For example, a naive git-blame-like feature
might look like:
{ "insert": "\n", "attributes": { "blame": { "hash1": { "author": "Alec Gibson", "timestamp": 1582123032, } } } }Implementation
This change adds support to all of
compose,diff,invertandtransformfor complex attributes.The implementation is recursive, and each depth of attributes is
treated similarly to the top level. In particular:
nullwill remove a propertynullArrays
Note that arrays are out-of-scope of this change. Their behaviour is not
well defined when composing deltas together, and they are treated as if
they are primitive values (ie they directly overwrite one another,
rather than attempt a deep change).
Quill integration
Quill will not natively support these changes, and further work will be
required if this capability is to be adopted there. Given that Quill v2
is currently in development, this may be a good time to do this work, if
desirable.
In particular:
format()methods need tocomposevalues rather than simplyoverwrite
merge()methods need to perform deep equality checks, rather thanobject reference checks
Note that Quill can currently support these complex attributes, but only
with blots that override the above methods.
Backwards compatibility
This change is not technically backwards-compatible. If anyone is
already using deeply nested attributes and relying on the current
all-or-nothing behaviour, then this change will break their code.