Deprecate DraftEntity before removing

[flarnie]

Summary

A previous PR had completely removed 'DraftEntity' the module and moved
control over entities into ContentState.1 This changed several public
APIs of Draft.js in a non-backwards compabible way.

To make it easier to migrate multiple use cases to the new Draft.js API,
and because the transition is too complex for a simple codemod, we are
releasing a version that supports both the new and the old API and then
later releasing a version that breaks the old API completely.

In order to renew support for the old API while also supporting the new
API, this PR

• restores the DraftEntity module and associated tests/mocks
• calls into DraftEntity methods under the hood of the new API

We tried to leave as much of the new API code in place and unchanged as
possible, so that things will be easier when we are fully removing
DraftEntity and switching to the new approach.

Test Plan:
This should pass unit tests and I did some manual testing.
Still TODO:

• manually test in other browsers (will do before landing)
• increase test coverage for 'convertFromHTML', which broke at one point even though tests were all passing. (follow-up PR)
• Add some 'deprecated_examples' that demonstrate we are still supporting the deprecated API, for manual testing of the old API. (follow-up PR)

Next steps:

• PR to add missing test coverage and examples
• A PR to add warnings and comments about the deprecation of the old
  API
• Add all these related PRs to 0.10.0 alpha version
• Add documentation about migrating from deprecated to new API
• Release 0.10.0; support both APIs
• Make PR that basically undoes this one, moving entity control into
  ContentState, and release that as version 0.11.0, with warning that
  it no longer supports the deprecated API.

[flarnie]

I want to tag @johanneslumpe but it doesn't seem to be letting me. But since he did the work originally to migrate us to the new API, would be good to get perspective on this.
Also want to tag @stopachka because he helped review the original PR that changed the API.
Last, tagging @spicyj because he's been heroically handling bugs and PRs lately.

[johanneslumpe]

@flarnie I'm not sure I agree with the new signature of the functions, where contentState is put last, after the callback.

The downside of having this intermediate deprecation phase in general is that it nullifies the gains of the original change, since the problem with entity changes not triggering a content state update will persist.

[flarnie]

@johanneslumpe you are right that this version won't actually give the benefits of the API change. That's why I hope to release the fully migrated version quickly after releasing this.

Version 0.10.0 -> warns when you use the old API, but doesn't have benefits of new API yet
Version 0.11.0 -> no longer supports old API, and has benefits of the new API

Because we have many and varied uses of Draft.js internally, it would be almost impossible to migrate all of them to the new API in one swoop. So updating to a version of Draft that doesn't support the old API would probably not happen, and I think we would fall out of sync with the open source version. Other folks using Draft will also find it easier to migrate this way too. And it lets us quickly release the other bug fixes that are present on master currently.

If we want contentState to not be the last argument, is the reasoning that we should ensure that people know that it's being passed in? In that case we could switch the order in version 0.11.0 when the API is no longer backwards-compatible. However, that means that even if someone does update all their use cases when switching to 0.10.0, they still have to change all their 'strategy' callsites to update to version 0.11.0.

Also - did we consider making the entity strategy arguments an object instead of three separate parameters? Three or more starts to get to be too many imo, and what if we want to add more in the future?

[johanneslumpe]

@flarnie The reasoning for the order of arguments is the "callback last" convention.

I agree that having a lot of arguments on a function call is not desirable and we could potentially move to an object here, if we don't see performance issues with it.

[flarnie]

The tests and flow are passing on my local machine. Looking into why they are failing in CI.

[flarnie]

It looks like the failing tests are unrelated and are happening on all recent PRs. Opened an issue and will look into it more tomorrow.

#799

[flarnie]

@johanneslumpe what would you think of doing the full change to the entity strategy arguments in 0.11.0, and at that point either putting the contentState as the first argument or moving all arguments into an object?

Hoping to get more feedback on this before landing - @hellendag or @spicyj any thoughts?

[johanneslumpe]

@flarnie so basically reverting this whole PR and then just converting the args to an object? Seems reasonable to me. At that point we probably should change all public apis which have multiple arguments in order to not have a mix of config objects and plain arguments. Thoughts on that?

[flarnie]

@johanneslumpe You have a point that if we make the multiple args an object here, we would want to do that everywhere. In that case I'm biased towards the smaller amount of change to the API, and I'd rather just change the order of the arguments to put the callback last at that time.

I wasn't familiar with that convention, but there is a reference to it in the node.js style guide. Seems reasonable.

[flarnie]

I'm going to land this since I have some of the follow-up work ready, and want to make a new PR with that. Happy to continue conversation about this API change process. I think this is cool though since @johanneslumpe has given some feedback and I talked to @hellendag about it a while ago.