[WIP] Moving entities into content state #185

[johanneslumpe]

This is just a very quick, hacky PR to showcase the general direction of how I want to approach things for #185. I mainly want to gather some feedback so we can discuss whether we want to keep DraftEntity and just use that to perform transactions on the state or whether we want to go a different route.

I have updated most of the tests to pass. One is failing due to me not having implemented DraftEntity.mergeData and two other ones are failing because they rely on an implementation which requires a contentState argument and I didn't refactor anything there yet.

I'm totally down to refactor anything here or to go a different direction. Just wanted to get the ball rolling. But one way or another: this is going to be a breaking change ;)

[amireh]

Status update possible? Is there anyway we can help?

Thanks for taking the initiative!

[johanneslumpe]

@amireh I am waiting for initial feedback from @hellendag regarding this.

[hellendag]

Might make sense for createFromBlockArray to accept a second parameter for the entity map, rather than requiring the set below.

[hellendag]

Thanks for your patience. :)

I think this is on the right track. A couple of comments here. Mostly I'm thinking we can just move everything directly into ContentState and throw out the DraftEntity module altogether.

[johanneslumpe]

@hellendag I was on vacation last week. I hope I'll be able to take a look at this soon! :)

[bkniffler]

Hey @johanneslumpe and @hellendag, this is awesome and in my opinion one of the more important things that need to be implemented atm. Not having entity state changes refresh the content state is a big pain point that gave me loads of headaches while developing plugins for draft-js-plugins, at the point were I change both component state and entity state to force atomic editor components to rerender, since forcing a rerender by setting selection doesn't work reliably.

Is there anything you need help with? Has there been any further discussions about this PR internally @hellendag?

[johanneslumpe]

@bkniffler Unfortunately I still didn't have time to revisit this - got some personal things to deal with right now. I will get back on this eventually though!

[johanneslumpe]

@hellendag the only thing left to fix is the DraftPasteProcessor or rather convertFromHTMLtoContentBlocks, which creates LINK entities. In order to do that I think we either have to give it access to the content state. But then again we won't be generating chunks/fragments but rather a new/updated contentState whenever we parse HTML. There is only a single call site to convertFromHTMLToContentBlocks though inside the DraftPasteProcessor and that again is only called in the editOnPaste handler:

if (html) {
      var htmlFragment = DraftPasteProcessor.processHTML(
        html,
        this.props.blockRenderMap
      );
      if (htmlFragment) {
        var htmlMap = BlockMapBuilder.createFromArray(htmlFragment);
        this.update(insertFragment(this.props.editorState, htmlMap));
        return;
      }
    }

So instead of having a fragment here, we could just return a new contentState and then call this.update with the new state. What are your thoughts on this? Do you see any issues with that?

Another approach would be to pass contentState, but only use it to register new entities and return both the updated content state and html fragment, so when calling insertFragment we can just pass an already updated EditorState instance to it, in order to have all new entity references work.

[johanneslumpe]

@hellendag I went ahead and committed 012ea41 - it's one way to do it. we're generating additional objects now when parsing html now, but I'm not sure that that's a huge issue. Let's discuss what you think about it :)

[hellendag]

As you know, I don't love boolean arguments and return values. :) But I want to get this unblocked, so we can just revisit if needed.

[hellendag]

Okay, let's go ahead and do this, then refine as needed.

Thanks again for your patience! :)