Update version of immutable

[icopp]

Summary

Updates the version of immutable that draft-js is using to 3.8.1 in order to remove unnecessary console logs (resolves #607). Note that a previous PR was closed because immutable-js/immutable-js#850 wasn't ready yet, but that PR is now finished.

Test Plan

Ran all tests to verify they still worked.

[williamboman]

This would be 🎉 in order to dedupe our immutable dependency.

[flarnie]

Thanks for updating this! The only issue is the failing flow/lint checks in CI, which don't seem related to your change.

If they fail again I'll try rebasing this branch and see if that helps.

[flarnie]

Hi again - I just pulled down your branch, rebased it, and ran lint and flow. Flow errors look legit - probably due to changes with the flow typing in Immutable.js v3.8.1.

Please fix the flow errors that come up when you run npm flow or yarn flow on this branch, and then I'll be happy to merge. Thanks!

[flarnie]

I'm going to take a shot at fixing the flow issues - either today or tomorrow hopefully will get this ready to go.

[flarnie]

There is still one flow error showing up for my locally that I wasn't able to find a fix for or silence. Would love help finding a solution - going to post more details about the problem and open an issue in the morning.

[flarnie]

So there were quite a few flow errors, and I'll start with a summary of the fixes that seem uncontroversial:

• Adding an invariant requiring that we find something when we look up a certain leaf in the editorState.blockTree.
  • It is possible for this value to be undefined, and in that case we are in a broken state and I think it makes sense to throw. It would lead to the 'start' and 'end' values being undefined, which would lead to buggy behavior in the editor, with ranges not being removed/modified correctly.
• Added another similar invariant in getUpdatedSelectionState. In this case the code would already throw if we get 'undefined' there, and the invariant makes it clear what we expect.
• In Immutable.js v3.8.1 there appears to be [a bug where an OrderedMap and OrderedSet get typed as a regular Map and regular Set](https://github.com/facebook/immutable-js/issues/979). I believe that it is fixed here and so until that fix is released in a stable version I silenced the resulting errors with a $FlowFixMe comment.
• In one case we loosened a type definition to use Set instead of OrderedSet to avoid the above bug until the fix is released.
• In removeRangeFromContentState we are trying to update the BlockMap. According to it's type definition, it's a map with strings for keys and ContentBlocks for values. There should never be a string key with a null value. However, in the past we removed a value in one place by calling .concat(Immutable.Map([[key, null]])). In the docs I didn't see a remove method for an Seq.keyed, which is what Flow thinks this data structure is. So I used a 'filter' call to remove the key value instead.

New changes were introduced - needs further review

[flarnie]

I'm going to get someone else internal to review this once we get Flow passing, since it's got more significant changes than just the dependency version bump.

The last flow error is:

Error: node_modules/immutable/dist/immutable.js.flow:475

475:   static <V>(obj?: {[key: string]: V}): Map<string, V>;

                                                 ^^^^^^ string. This type is incompatible with

 24: export type DraftBlockRenderMap = Map<DraftBlockType, DraftBlockRenderConfig>;

                                           ^^^^^^^^^^^^^^ string enum. See: src/model/immutable/DraftBlockRenderMap.js:24

I don't want to loosen the type definition for DraftBlockRenderMap but it also looks like a string enum is not compatible with a string type requirement. This is not a bug in Flow, but how things work by design.

I have tried reformatting the DraftBlockType enum to follow different patterns, including the $Keys pattern, and various solutions proposed on this issue, but that still doesn't satisfy this error.

I'm going to file an issue with Immutable.js and see if anyone there has advice about how to mitigate this error.

[flarnie]

Unfortunately this doesn't provide the same functionality as the previous version. Going to silence the error and open an issue to fix some of these 'flowfixme' comments.

[facebook-github-bot]

@flarnie is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[facebook-github-bot]

@flarnie has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[facebook-github-bot]

@flarnie has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[facebook-github-bot]

@flarnie has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[flarnie]

There are two new Flow errors internally. I am guessing our Flow version/configs are slightly out of sync with external, and our version of Immutable.js may also differ internally. ☔️

I'll try to resolve them tomorrow.

[flarnie]

Just waiting on the PR within Facebook's codebase to be approved and landed which will fix flow errors resulting from this change, then we can land this. 🤞

[flarnie]

Hate to go back on this after so much work to get Flow passing, but we realized this:

• this is a breaking change to Flow types, and required changes to our Draft.js callsites internally.
• we should wait to ship any breaking changes to Flow types until 0.11.0 or even 0.12.0, and it's not clear what version of Immutable we should jump to in order to minimize breaking changes.

I am hoping we can work on updating to support Immutable.js 4.* - see immutable-js/immutable-js#1388 (comment) and #1425