Test: Move the merge block handler to the actions file and unit test it

[youknowriad]

I'm planning to work on the "forward merge", before this, I've moved the mergeBlocks action creator into actions.js and unit test it.

Side note: We start having action creators that take "dispatch" as a parameter and others that return actions, we should probably decide to include thunks or an alternative soon.

Edit: The last commit adds forward deletion and should close #506

[aduth]

I really don't like the idea of Editable assuming a context with the editor's store, but it sure would be nice if onMerge here were the connected dispatch, wouldn't it? 😄 Since it means blocks wouldn't need to pass this through at all.

[youknowriad]

Yes, but it's not that simple. Imagine a block having multiple editables. So the merge of the last one would probably merge its content with the previous one (or do nothing) but the merge of the first editable would actually trigger the merge with the previous block.

It could be way simpler if it's one block = one editable, but that's not the case. Could we provide more "metadata" on how the block uses the editable in this case and fallback to one editable mergeable with the previous block if this metadata is not provided? I don't know but that's something we should think about.

example:

edit() {
  return (
    <div>
       <Editable value={ attributes.content } mergeWithPrevious />
       <Editable onMerge={ mergeWithAttribute( 'content' ) } />
       <Editable noMerge />
    </div>
  );
}

[aduth]

Imagine a block having multiple editables.

Ah, that's a good point. I hadn't considered this.

Could we provide more "metadata" on how the block uses the editable

Interesting idea. I think at the point where we're saying a block implementer needs to be concerned with merge behavior (and it's fairly obvious they do), I'm not as much concerned about accepting and applying the onMerge prop handling anymore.

It could be slightly more optimized if Editable were able to handle this by hints, but I'm not sure a clean way we can connect the Editable to the editor's store without tying it directly to the editor (not so great).

[aduth]

To your points here, do you think it's obvious to the block implementer what's happening when they pass mergeBlocks to an editable, specifically in merging with a previous block, next block, or between editables of the same block?

[youknowriad]

I think this should be well documented. It's not obvious at all for now but I don't have any alternative. We should enhance our block API documentation once it's more stable.

[aduth]

cc @mtias since we'd chatted about this topic today. Maybe for the sake of simplicity we don't allow complex merging within a block, and try to make editor smarter about knowing how to handle Editables. It's a very fuzzy desire, but maybe traversing returned element to find first or last editable to decide how to merge into previous or next. @mtias indicated some desire to avoid merging and transforms being responsibility of block at all, which sounds nice; difficult to pull off 😄 But I think it'd be fair to make some compromises about what merging can be performed if it helps simplify.

I don't think this needs to be settled here; in fact, mergeBlocks could very well be compatible with this future desire. Just a matter of using context to communicate directly between VisualEditorBlock and Editable to avoid block needing to handle directly.

[mtias]

I missed this comment @aduth. Let's create an issue for this so we don't lose sight of it.

[aduth]

I missed this comment @aduth. Let's create an issue for this so we don't lose sight of it.

See #878

[aduth]

Side note: We start having action creators that take "dispatch" as a parameter and others that return actions, we should probably decide to include thunks or an alternative soon.

Related: #691

Do you have any opinions on this? I've actually been leaning toward thunks a bit; even if I personally dislike their data "impurity", it's hard to argue that they're not simple to use, which could be a big win for what's already a complicated setup.

I'll plan to do a proper review of the changes here shortly.

[youknowriad]

@aduth I have a personal preference for generators, redux-saga is a bit complex though, so I use a personal library in general https://github.com/youknowriad/redux-rungen. But this is a lot to handle I think, requires a certain mastery of generators. Thus, I'm also leaning towards thunks to avoid having a steep learning curve.

[aduth]

This looks good 👍

[aduth]

Similar to what we did with concatChildren, I wonder if this needs to be necessarily limited to merging two blocks (unless it becomes much more complex).

[youknowriad]

Yes not as easy as concatChildren, because merging blocks could produce several blocks, merging only the first paragraph. I'm thinking we could do this recursively, but I don't see much value to support this for now.

[BE-Webdesign]

Do you have any opinions on this? I've actually been leaning toward thunks a bit; even if I personally dislike their data "impurity", it's hard to argue that they're not simple to use, which could be a big win for what's already a complicated setup.
I'll plan to do a proper review of the changes here shortly.

I can see refx being more valuable than thunks. Thunks can pin you into a corner when you want to have dispatched actions trigger other side effects, rather than having whole new action creators added to the codebase. By using a refx-esque approach you can incrementally build complexity on top of the action flow already happening. When auto drafting is implemented, that would be somewhat tricky to do in thunks, but pretty easy to manage with refx. The downside is that it might be more hidden as to what is actually happening when using a library like refx, as the flow of actions is no longer hard coupled to the action creators' functions. Even considering that, I think refx is the right choice, as it will be more flexible and easy to use and maybe the code can document that refx side effects are occurring. If a developer is new to Redux, thunks and refx will both be foreign, so trying to pick a simple to use solution won't provide much benefit to them in my opinion. Even though thunks are more commonplace (probably who knows), I think refx is a better choice and is equally simple to use.