Slate: #2619 seems an incomplete merge

[bebbi]

Do you want to request a _feature_ or report a _bug_?

bug

I got bitten by #2619, I think it's missing a Changelog entry for that merge.

As I was considering one, I noticed a few issues:

1. When I am adding an array to a node.data key using node.data.set(myKey, myArray) it appears this is now deeply converted to immutable.

However, when I then request a serialization of the whole tree using value.toJSON() it does not convert this back to json but keeps my array a List which is unexpected.

1. I then checked the corresponding issue and it doesn't show consensus on the merge - I also tend to agree with the opinion that storing things in node.data should be up to the user?

cc'ing contributors to that issue there:
@thesunny
@ericedem
@ianstormtaylor
@delijah
@benjycui

[thesunny]

Yep, I merged this but I think we should revert it.

My original thinking was that everything under an ImmutableJS type should also be ImmutableJS. Not doing this seems like an anti-pattern and a bug so enforcing the conversion and making it simpler made sense to me; however, I missed the (compelling) argument on the issue that since we are moving to ImmerJS, we would want to allow using plain JS objects. This makes sense to me.

It still feels weird to me to allow plain JS objects under an ImmutableJS object, but I think the argument above supersedes that.

So, I flubbed this one because.

• It's a breaking change
• Once we use ImmerJS the anti-pattern would be the right way so we should allow it

I'll leave it here for a few days in case there are any other arguments for/against. Otherwise, I'll revert.

[thesunny]

FYI, reverted here

https://github.com/ianstormtaylor/slate/pull/2687