Draft-js: Immutable-js newest version -> draft-js incompatible

[markpradhan]

What happened

I updated to 4.0.0-rc.7.
Suddenly i noticed draft-js stopped working. Threw an unrecoverable error.

How to reproduce

Install draft-js 10.3, install the version of immutable i mentioned. It will happen naturally.

I also posted on immutable

[flarnie]

This is a valid issue - first we should land https://github.com/facebook/draft-js/pull/1289 and then I'd love it if someone picked up this.

[flarnie]

PS - to get Draft compatible with the latest Immutable.js will probably require some Flow fixes.

[acusti]

@flarnie I created a new branch that brings in immutable 4.0.0-rc.7 and fixes the test failures it caused. I then ran npm run flow and got 172 errors. Seeing your comment above about #1289 and the large number of flow fixes and changes included in that PR, I pulled icopp:master down, merged in facebook:master, and tried basing the upgrade commit off of that. However, that leads to 180 errors from flow.

My team recently went through a pretty epic effort to upgrade to immutable 4 in our own codebase and resolve all the flow issues that resulted. One thing we ran into and that I also saw in the results of running flow was that in order to resolve a number of issues, we needed to upgrade flow to at least 0.55.0. Reference: https://github.com/facebook/immutable-js/issues/1360

After upgrading flow to latest (0.57.2), and continuing with the branch I made based off of #1289, the total number of flow errors is 163. Does that seem like a good place for me to start? I am very eager to help with this issue to get our stack updated. We‘ve been running type-blind (without flow) for a couple weeks due to issues with immutable v3.x types. Hope I can make this happen.

[flarnie]

Thanks so much @acusti! I have not yet looked into why there are so many Flow errors. What you described does seem like a good place to start, and I wonder what the root cause of the errors is.

Once we figure out the cause(s), if there are things in Immutable.js that could be adjusted I think @leebyron is open to those suggestions, if we can make the upgrade path smoother.

[sophiebits]

Upgrading Draft to use Immutable 4 is a breaking change for users of Draft, correct? Since EditorState etc expose the 3.x API. (Haven’t looked at what’s new in 4.)

[acusti]

@sophiebits That’s an interesting question. Internally, draft’s use of immutable had to be adjusted in only one place: src/model/immutable/ContentState.js#104 (https://github.com/brandcast/draft-js/commit/49e5ccd0e01f226c03643e1d909484b91d809555#diff-828258081fbb9e6fd8c336f3140f0a2eR104), and the change, as recommended in facebook/immutable-js#1340, is backwards-compatible with v3. So, with that in mind, the dependency range I specified for immutable is ^3.8.1 || 4.0.0-rc.7. I also had to make the same backwards-compatible change in a test file, but I’m most interested in code that ships to users of Draft.

Which brings up the second issue: the improved flow types in immutable v4 are of course not backwards-compatible with immutable v3, so in terms of types, these changes will require immutable v4, which as you mentioned, would represent a breaking change for users of Draft. Hence why I used the dependency range above that includes ^3.8.1, but I imagine doing so will cause issues with CI or with anyone who has a clone of the repo and who tries to npm install, and winds up keeping 3.8.1 and gets hundreds of flow errors as a result.

[acusti]

@flarnie In our case, a good number of the new flow errors we got were for unsafe operations that previously were not accurately typed. For example, fromJS will not necessarily return an immutable Collection (formerly Iterable), e.g. fromJS(null) === null, but previously, you could do const thing2 = fromJS(things).get(1); without getting a flow error.

[acusti]

@sophiebits Just realized that I haven’t been considering users of Draft who consume it via a script tag pointing to the browser bundle, but looking at the Readme, it seems like that is no longer an officially supported distribution method?

[acusti]

@flarnie @sophiebits By far the biggest source of new flow issues after upgrading to immutable v4 are legitimate errors related to nullable values from invoking get() on immutable objects. For the majority of those issues, I have been relying on nullthrows, operating under the assumption that the value must be present and that a null value indicates a broken editor state.

Specifically:

https://github.com/facebook/draft-js/blob/master/src/model/immutable/ContentState.js#L66

ContentState’s getBlockForKey is currently incorrectly annotated as getBlockForKey(key: string): ContentBlock {. This is incorrect both because this.getBlockMap().get(key) could return undefined and because that behavior is enforced via the tests. I updated all instances where getBlockForKey() is being invoked and relied on to return a ContentBlock to have a nullthrows() invocation around that statement. For example, in moveSelectionBackward.js#44:

      var blockBefore = content.getBlockForKey(keyBefore);

becomes:

      var blockBefore = nullthrows(content.getBlockForKey(keyBefore));

This works and preserves the actual (as opposed to documented) current functionality, but I’m not sure that it’s the best. I didn’t come across an instance in the codebase where the fact that getBlockForKey could return undefined was used, and I did notice that in most places in the code, as well as in the docs, the method is expected to always return a value. So, I could take an alternative approach and instead add an invariant to that function to ensure that it returns something. I would then update the Content State > block fetching test to expect the function to throw, rather than return undefined, when getBlockForKey is invoked with a key that doesn’t match anything.

The third alternative approach would be to return an empty content block when the key doesn’t match anything. The only argument for that approach that I could think of would be to try to follow the robustness principle (Postel’s law) with the public-facing API of ContentState.

The same question applies to the getSelectionBefore and getSelectionAfter methods; in that case, I converted:

  getSelectionBefore(): SelectionState {
    return this.get('selectionBefore');
  }

to:

  getSelectionBefore(): SelectionState {
    const selection = this.get('selectionBefore');
    invariant(selection, 'ContentState missing selectionBefore');
    return selection;
  }

Generally, I’ve been trying to apply a decision tree along the lines of:

1. If we are confident a (theoretically) nullable value is not null (e.g. calling first() on a List instance where we have already confirmed that its size is 1 or more), I use nullthrows() around the method invocation
2. If a nullable value being undefined is an indication of a bad state for the draft editor instance as a whole, I use invariant (or nullthrows, if it doesn’t seem to make sense or be useful to throw a specific error message)
3. If it seems like the code in question can execute with the nullable value being undefined, I add a simple guard

It seems like that largely matches how those techniques are already used. The trouble is that, as in the above examples, it is often hard for me to deduce which case is most accurate in that decision tree. Here’s an additional example where I chose the third option above:

In keyCommandPlainDelete.js#34:

      var charAhead = content.getBlockForKey(key).getText()[offset];
      return moveSelectionForward(
        strategyState,
        charAhead ? UnicodeUtils.getUTF16Length(charAhead, 0) : 1,
      ...

Because the code already checks if charAhead is truthy, I changed it to:

      var block = content.getBlockForKey(key);
      var charAhead = block && block.getText()[offset];
      return moveSelectionForward(
        strategyState,
        charAhead ? UnicodeUtils.getUTF16Length(charAhead, 0) : 1,
      ...

I can push up that work for review if useful, but I wanted to get opinions on the above questions first before committing with the intention of getting whatever I do merged.

[acusti]

I pushed up the rest of the commits. All test are passing ✅, and there‘s only one flow error left, which is this one: https://github.com/facebook/immutable-js/issues/1417

Commits of note:

• 56f3ef2c1a1c9dda691c052f61b3b9d2c88cb5b6 corrects ContentState getBlockForKey’s return value to mark it as nullable; this matches the existing behavior of that function, but as I mentioned above, there are a couple other approaches that could be taken, though they would require changing how that method works
• bf6723af8dc7e54b8666aeeeed0acb2cd3747f28 takes a different approach to the other parts of the ContentState API that were not type safe, because the other methods don’t require finding a key that gets passed in. So, I added invariant calls to ensure that the required content is always present.
• e0676d0dfd5b0360be24d8a8db1237f1e397ddd8 takes a similar approach as the uses of invariant in ContentState, but uses nullthrows instead, because there where many more instances and it was easier and required less code

[flarnie]

So awesome to have your help on this! I'll look it over in a bit - I think we might need to do a 0.11.0 release for this, since it may be a breaking change.

[acusti]

Sounds great! In order to test it out locally, you’ll need to use the latest immutable, which hasn’t yet been released. Here’s how to install it:

npm i immutable@facebook/immutable-js#npm

Also, I pushed up a package.json for trying to see if this could be a non-breaking change: 12b58bf

The idea is that it updates the direct immutable dependency to require v4, but uses peerDependencies to include v3, so that draft users who are still on immutable v3 can remain there. I believe all the code changes I made are backwards-compatible, though the flow type changes, of course, are not, so if any of the users on immutable v3 were relying on draft-js’ flow types (as opposed to ignoring them in their flowconfig, I expect they will get a lot of new flow errors, but I don’t know if you all would consider that a breaking change. I should also point out that I don’t really understand peerDependencies, so that might not be the correct usage of it. And lastly, for ContentState.getBlockForKey, I changed the docs to specify that the returned value is nullable, but that’s actually not a change in the API, only a correction in the docs.

@sophiebits on the above proposal and questions around if this upgrade needs to be a breaking change.

[fabiomcosta]

Any chance we'll ever get to at least update to the latest 3.x.x version? :/

[Gregoor]

6 months seem sensible for another ping. Any interest from the Facebook side to look into this? Looks like there was a contributor interested in helping getting this over the finished line, but that ball might've been dropped already?