Upgrade Immutable to 3.8.1

[djforth]

Do you want to request a feature or report a bug?
Feature

What is the current behavior?
Current versions of Draft use Immutable 2.7.4

What is the expected behavior?
Would be useful to upgrade to latest version (3.8.1) to reduce dependancy mismatch (Working on project that needs 3.8.1, which means I need 2 versions of immutable). Also is worth setting Immutable as a peer dependancy rather than a dependancy so people are aware if there is a mismatch.

Which versions of Draft.js, and which browser / OS are affected by this issue? Did this work in previous versions of Draft.js?
Had a quick look and 0.91, 0.10-stable & 0.10-alpha all depend on 2.7.4

[Bleeky]

Completely agree, would be nice to have such an update of the dependencies. Plus, the #607 is still open, and this can cause some serious problems on some browsers.

[djforth]

I did have a quick look at upgrading it fork, but looks like it isn't a trivial task. Would be happy to have a go if the maintainers think it would be a useful thing?

[quicksnap]

Related to #879

[chrisui]

Could we declare Immutable as a ^ version rather than ~ so that we can be on any later minor version rather than a specific one? This would allow us to upgrade minor versions in our apps without having to get a new version of draft-js every time.

[flarnie]

This would be great! If anyone wants to work on it, we'll make time to review the PR.

[RomanLysogor]

Hey folks, any updates on this one? I found this issue as a performance critical. The logging message appears on each user typing, thus is slowing performance significantly.

[mc-funk]

Seconded that an update on this would be helpful. The logging messages are really detrimental.

[octatone]

Hi there, I just discovered that my bundled code contains two copies of immutable because of draft-js' old version dependency. We are using [email protected] in our code, [email protected] is [email protected].

There are fixes for immutable Records between these two versions that we assert in tests in our code and that which would improve performance of draft-js as a whole.

[shakdoesgithub]

Hi, wanted to see if there are any plans on doing this? As others have said, the console.warn is on every input changed and causes serious slowdown.

[mitermayer]

Hi @shakdoesgithub ,

There is an open PR for bumping to immutable 4 on #1439 however if you would like to work on the meantime to bump to 3.8.1 that would be awesome as well

[CarsonYong]

As previously mentioned, if you're running into this issue with the hundreds of console.warn and the serious degrade in performance because of it. One simple fix is to fork draft-js and include the fork into your project. I have a fork of draft js with version 3.8.1 of Immutable, that anyone can use if they like until an updated version of Immutable is added to draft-js.

[mc-funk]

@CarsonYong Perhaps I'm missing some context here, but would it be worth it to try submitting your bump as a pull request?

[TomiS]

The work was enormous, but somehow I managed to pull it through ;)
#1573

Edit: Oh, crap, it's failing flow type check. I'll take back everything I said.

[mc-funk]

@TomiS we're all rooting for you!

[pfarnach]

@TomiS is there any update on this? I'm getting loads of warning messages using Immutable via DraftJS.

[TomiS]

Not really much to say. There are ~20 flow errors to solve after updating immutable to 3.8.x. Some of them seem quite simple but there is one problematic:

Error: node_modules/immutable/dist/immutable.js.flow:406
406:   static of<T>(...values: T[]): SetSeq<T>;
       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ function type. This type is incompatible with
387:   static of<T>(...values: T[]): IndexedSeq<T>;
       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ function type
  This parameter is incompatible:
    406:   static of<T>(...values: T[]): SetSeq<T>;
                                         ^^^^^^^^^ SetSeq. This type is incompatible with
    387:   static of<T>(...values: T[]): IndexedSeq<T>;
                                         ^^^^^^^^^^^^^ IndexedSeq

It looks like an internal type error in immutable.js that probably cannot be fixed outside that lib. I can get rid of that error by setting
.*/node_modules/immutable/dist/immutable.js.flow
in the ignore section of .flowconfig but all the other errors also disappear which is probably not cool.

[pfarnach]

Ah seems annoying. I have zero experience with flow, otherwise I'd lend a hand. Are there any maintainers (@flarnie) that could jump in to help?

[betancourtl]

Did this ever get fixed? I'm getting flooded by these messages making it hard to see relevant logs. Feels like a DDOS attack on my console.

[hxgdzyuyi]

Did this ever get fixed?

[ddelrio1986]

If you add your own dependency for a newer version of Immutable the errors should go away. I have this in my dependencies:

"immutable": "^3.8.2",

[maiah]

Adding immutable 3.8.x in your own dependency doesn't work. We really need to upgrade the version in draft.js.

[TomiS]

So, I took another attempt on this. With the latest flow-bin (0.70) the errors already look quite nice. However, I encountered this:
immutable-js/immutable-js#1510

Btw. As a workaround, in our app we did this (in package.json, using yarn):

  "dependencies": {
    "immutable": "3.8.2"
  },
  "resolutions": {
    "draft-js/immutable": "3.8.2",
    ...add all your plugins here like this:
    "draft-js-mention-plugin/immutable": "3.8.2"
  }

Seems to work (fingers crossed)

[TomiS]

If I run flow in draft-js codebase with immutable 3.8.2, I get total of 34 flow errors.

• 4 of them were there already before the update (relate to somehow flawed? require statements)
• ~15 are related to 3.8.2 toOrderedMap and toOrderedSet incorrectly flowtyped immutable-js/immutable-js#1510 and are expected to disappear when/if this is fixed
• One tricky immutable related:

Error ┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈ node_modules/immutable/dist/immutable.js.flow:406:33

Cannot extend super of SetSeq [1] with SetSeq because SetSeq [2] is incompatible with IndexedSeq [3] in the return value
of property of.

 [3] 387│   static of<T>(...values: T[]): IndexedSeq<T>;
        :
     401│   static of<T>(...values: T[]): IndexedSeq<T>;
     402│ }
     403│
 [1] 404│ declare class SetSeq<T> extends Seq<T,T> mixins SetIterable<T> {
     405│   static <T>(iter?: ESIterable<T>): IndexedSeq<T>;
 [2] 406│   static of<T>(...values: T[]): SetSeq<T>;
     407│ }
     408│
     409│ declare class List<T> extends IndexedCollection<T> {
     410│   static (iterable?: ESIterable<T>): List<T>;

In our app, we are using a variant of immutable flow typings where this line:
https://github.com/facebook/immutable-js/blob/v3.8.2/type-definitions/immutable.js.flow#L406
is commented out. I think it solves that issue.

All the others are errors originate from the draft-js codebase and should be relatively easy to fix. So I think with little effort this update should be really doable. But it would require a few batch fixes to immutable 3.8.x typings.

[paulyoung]

I was checking for progress on this issue and stumbled upon this workaround: #1647 (comment)

I had to specify "immutable": "^3.7.6" in package.json in my case to ensure a version supporting the option was used.

[macrozone]

@TomiS so flow errors in draft-js itself is holding it back from beeing fixed? so immutable 3.8.x basically works with draft.js?

[TomiS]

@macrozone We've been successfully running draft-js by forcing Immutable.js 3.8.2 (with resolutions field in package.json). So the code seems to work afaik. I don't think the flow errors can be fixed on draft-js level other than by ignoring them as the most troublesome of them originate from Immutable.js typings.

[macrozone]

i can also confirm that it works with immutable 3.8.2

[dchambers]

Presumably because our code is within a monorepo, but I had to modify the Yarn resolutions config to:

"resolutions": {
    "**/draft-js/immutable": "3.8.2"
}

to force Yarn to use version 3.8.2.

[kylebebak]

This is just crazy.

The issue is more than 2 years old, this library is downloaded more than a million times a month, and still no one has been able to upgrade one of its dependencies one minor version!!!

[claudiopro]

Hi @kylebebak, thanks for bringing this up again. Let me provide a bit of context as to why this is not as simple as upgrading a dependency one minor version. Draft.js is used internally at Facebook and is depending, like the rest of the www codebase on a version of immutable.js pinned to [email protected].

As @flarnie pointed out in #1289, upgrading Draft.js to immutable@^3.8.2 would require changing a number of internal Facebook callsites using Draft.js to the new 3.8.2 API. Things get further complicated as some callsites are using both immutable.js and Draft.js, and product code must also be refactored.

While I agree it is desirable to upgrade immutable to 3.8.2 in Draft.js for a variety of reasons listed by many contributors in this and other issues, this may not be an immediate win, and the maintainers' focus is currently to ship a new release to npm.

We will update this issue as the situation evolves.

[kylebebak]

@claudiopro
Thanks for this thorough explanation!

I did what @dchambers and other suggested and put 3.8.2 into my package.json resolutions, and everything in draft-js still worked, but no more cascade of identical warnings.

That was an interesting experience, it gave me the first true reason I've had to use yarn, because npm doesn't understand resolutions.

[ddelrio1986]

@kylebebak that must've been why it worked for me originally since I use yarn.