Draft-js: Move Entity information into State

[thesunny]

Related to this thread:

https://github.com/facebook/draft-js/issues/129

@angelf noted the weakness in using Entity objects, in particular they are not properly handled with Undo.

Moving Entity data into the ContentState should solve this issue.

Furthermore, in the referenced discussion, entity data is now also associated with ContentBlock information to store block data. This is in the associated PR.

https://github.com/facebook/draft-js/pull/157

By moving Entity into the state, we could do things like store depth information for lists in the Entity data and have it work with undo.

Feels like there are a few ways to move Entity data into the ContentState. Just to start the discussion.

1. Entity information can be stored as part of the ContentState.
2. Entity information can be stored as part of the ContentBlock (inside the ContentState).

The benefit of 1. is that it should behave fairly similarly to the existing implementation except the Entity data would be stored as part of the ContentState. I'm supposing one main difference is that there would have to be cleanup done on the entity objects within ContentState such that when an entity is no longer used, it needs to get deleted. Haven't looked at the implementation in Entity but I'm guessing that when using Entity the references are just kept around and "cleaned up" during the convertToRaw (i.e. unused entities are not part of the export).

The benefit of 2. is that cleanup could operate on much smaller chunks (a block at a time). For example, if a ContentBlock is deleted, all of the entities disappear with it so no additional cleanup work needs to be done. There would still need to be some cleanup if, for example, a link is deleted within a block, then the Entity is removed from the block. However, we don't need to look through the entire ContentState to do the cleanup.

[hellendag]

I'm not really worried about entity cleanup, especially since entities will be present in memory for ContentState objects in the undo stack anyway, whether they're on the ContentState or the ContentBlock.

I would be in favor of putting entities in ContentState rather than ContentBlock. A simple reason that comes to mind: if I want to split a block, it also means I'd have to split entities into their appropriate blocks as well. Or if I want to move a range of text from one block to another, I'd have to move any entities along with it. If the map lives higher up in the ContentState, then there is no added complexity -- we can continue referring to entities purely by key.

Overall, though, I am in favor of a change. The global map made sense for the original pre-immutable model, but is now mostly a hindrance.

[thesunny]

That's a good point about splitting blocks that I didn't think about.

Also, I guess we would still need the convertToRaw methods anyways and the cleanup could be done during that time so that unused entities aren't saved.

[hellendag]

Right. At conversion time, we'd just ignore any entities that aren't actively in use among the blocks.

[johanneslumpe]

@hellendag Have you thought about an api for this? Are we keeping Entity.create and maybe just additionally pass contentState to it, so the entity can be added to the entity map of the current content-state? Or do you have different plans?

What are your thoughts on a method, which encapsulated creation and application of entities, basically a boosted version of Modifier.applyEntity:

const contentStateWithEntity = Modifier.createAndApplyEntity(
  currentContentState,
  targetRange,
  'ENTITY_NAME',
  'MUTABLE',
  { some: 'data' },
);

If we wanted to retrieve the auto-generated entity key we could do so like this:

contentStateWithEntity.getEntityMap().last().getKey();

The method would update the content-state to include a new entry in contentState.getEntityMap(), then use Modifier.applyEntity to apply it and return the new state.

[coopy]

This enhancement would fix this issue:

When importing HTML containing <a> tags via DraftDraftPasteProcessor.processHTML, the LINK entities get parsed, but seemingly on the wrong Entity object.

When a decorator with a strategy like the one in the link example then is used to create the editorState from contentState with EditorState.createWithContent(contentState, decorator), the entity keys do not reference the parsed entities.

The workaround is to parse the contentState twice:

// Parse once to generate entity instances
EditorState.createWithContent(contentState);
// Now we can add our decorator
return EditorState.createWithContent(contentState, decorator);

[johanneslumpe]

I will start on this on the weekend probably.

[qrohlf]

This enhancement would also fix this bug:

Currently, if you modify an Entity using Entity.mergeData without changing any content, you have to manually force the Editor to re-render by making some arbitrary change to contentState or editorState. It's not enough to force the containing component to re-render, since the current implementations of shouldComponentUpdate in DraftEditorContents and DraftEditorBlock do not perform checks on the global entity store.

[johanneslumpe]

@coopy @qrohlf @bryanrsmith @jacobcarpenter @thesunny There is a first working version of this here: https://github.com/facebook/draft-js/pull/376 feel free to give it a try to see if it solves the bugs you have been encountering related to the global entity map.

[mitermayer]

Closing this since https://github.com/facebook/draft-js/pull/376 tackles it

[moontails]

Could anyone please recommended a workaround to make updates to entities (until v11.0 is out)?