consider migrating from Immutable.js "Records" to plain objects

[ianstormtaylor]

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

Discussion.

What's the current behavior?

When Slate was first created, Immutable.js was the best and most popular way to handle immutability in React. However, it has many downsides for our use case:

• CON: It requires a fromJS step to build the collections/records. Since the objects aren't the native JavaScript data types you get from JSON, we have to have an interim step that instantiates the Immutable.js data model. This can be costly for large documents, and there is no way around it. This is especially problematic in server-side environments where serializing to/from JSON blocks Node's single thread.

• CON: Reading values is more expensive. Immutable.js is optimized for non-crazy-slow writes, at the expense of read operations being much slower than native JavaScript. Since Slate's model is a tree, with many node lists, this ends up having a significant impact on many of the common operations that take place on a document. (See this question for more information.)

• CON: It introduces a fairly steep learning curve. People getting started with Slate often get tripped up by not knowing how Immutable.js works, and its documentation isn't easy to understand. This results in lots of "wheel reinvention" because people don't even realize that helper methods are available to them.

• CON: It makes debugging harder. In addition to a learning curve, debugging Immutable.js objects adds extra challenges. There's a browser extension that helps, but that's not good enough in lots of places, and you end up having to us toJS to print the objects out which is very tedious.

• CON: It increases bundle size. The first increase in size comes from just including the immutable package in the bundle at all, which adds ~50kb. But there is also a more insidious bundle size increase because the class-based Record API encourages monolithic objects that can't be easy tree-shaken to eliminate unused code. Whereas using a utility-function-based API, similar to Lodash, would not have this issue.

However, it does have some benefits:

• PRO: Custom records allow for methods/getters. This has been the primary way that people read values from Slate objects. Because Slate (and rich text in general) deals with some complex data structures, having common things packaged up as methods allows us to reduce a lot of boilerplate and reinventing the wheel for common use cases.

• PRO: It offers built-in concepts like OrderedSet. These allow for more expressive code in core than otherwise, because we'd need to reinvent the wheel a bit to account for JavaScript not having some of these concepts. Although truth be told this is probably a fairly minimal gain.

---

Since Slate was first created, immer has come on the scene (thanks to @klis87 for kickstarting discussion of it in #2190) which offers a way to use native JavaScript data structures while maintaining immutability. This is really interesting, because it could potentially have big performance and simplicity benefits for us.

All of the CON's above would go away. But we'd also lose the PRO's. That's what I'm most concerned about, and what I'd like to discuss in this issue... to see what a Slate without Immutable.js might look like, and how we could mitigate losing some of its benefits.

Without the ability to use classes and prototypes for our data models, we'd need to switch to using a more functional approach—exporting helpers in a namespace, like we currently already do for PathUtils. One question is whether this will be painful...

Looking at our rich-text example, we'd need to change how we do things in several places.

slate/examples/rich-text/index.js

Line 54 in 8eb8e26

return value.activeMarks.some(mark => mark.type == type)

We no longer have getters on our models, so value.activeMarks doesn't work. Instead, we'd need to change this to:

return Value.getActiveMarks(value).some(mark => mark.type == type)

Similarly, there's no value.blocks any more:

slate/examples/rich-text/index.js

Line 66 in 8eb8e26

return value.blocks.some(node => node.type == type)

So we'd need:

return Value.getClosestBlocks(value).some(node => node.type == type)

This is actually nice because we're no longer using potentially expensive getters to handle common use cases—calling functions is more clear.

But we also can't use helper methods like document.getParent.

slate/examples/rich-text/index.js

Line 148 in 8eb8e26

const parent = value.document.getParent(value.blocks.first().key)

Instead we'd need to use:

const blocks = Value.getClosestBlocks(value)
const parent = Node.getParent(value.document, blocks[0].key)

Similarly, we can't do:

slate/examples/rich-text/index.js

Lines 293 to 295 in 8eb8e26

	const isType = value.blocks.some(block => {
	return !!document.getClosest(block.key, parent => parent.type == type)
	})

And would have to instead do:

const blocks = Value.getBlocks(value)
const isType = blocks.some(block => {
  return !!Node.getClosest(document, block.key, parent => parent.type == type)
})

But we also get to remove some expensive code, since we don't need to deserialize anymore:

slate/examples/rich-text/index.js

Lines 41 to 43 in 8eb8e26

	state = {
	value: Value.fromJSON(initialValue),
	}

That's all for the rich text example, but it would definitely be a big change.

---

I'm curious to get other folks's input. Are there other PROS or CONS to Immutable.js that I haven't listed above? Are there other ways of solving this that I haven't considered? Any thoughts! Thank you!

[Nantris]

Wow that's a tough call...

For what it's worth, the Immutable Formatter extension doesn't work in Electron, so Immutable does make debugging quite a bit more difficult as I have to use toJS() constantly while debugging, but that's definitely far from a primary concern.

[ianstormtaylor]

@slapbox yup, very tough call. If we were starting fresh it seems like it would be a no-brainer, but hard to gauge benefits vs. costs now. Thanks for the input though! Anything is helpful, just trying to kick around ideas.

[PierBover]

I’m no expert in Slate, but I like the changes you’ve posted to the rich text example.

Personally I think removing dependencies and going closer to the metal is good for a project like Slate.

I can’t help with the codebase but I could help with the docs and examples if it’s worth anything.

[gersomvg]

Personally the learning curve for Immutable wasn't that steep. I must say that I have already scratched the surface of Immutable a few years back, and then decided to not use it, so that may have eased my learning curve.

But now that Immutable comes in a package deal with Slate, I find myself using quite some Immutable functions already, like value.document.nodes.get(-2) to get the second last node. This can also be achieved with (slightly more verbose) plain javascript or an imported utility library but it would definitely be quite a rewrite. I expect I'm not the only one who would need to rewrite quite some bit.

But I completely agree with the CONs. Especially the argument that by forcing people, that are not familiar with Immutable, to use it anyway, they will probably not use 95% of its functionality. One of the reasons I fell in love with the React ecosystem, after working with Angular, was that you needed to include small libs for almost everything yourself, so I could pick only what I thought was best for the project.

On one hand I'm wondering if this change is really needed on the short-term, since it will probably delay v1 by another few months, without really adding new functionality (rather removing it). But on the other hand, my fear of needing to rewrite quite a bit of code only supports making this change now, instead of in between v1 and v2, because then it might impact even more projects in production.

[skogsmaskin]

In my opinion we need to focus on stabilising what we have now. There has already been two heavy refactors lately. Those were needed and gave very clear benefits right away. This one I'm not so sure about (I agree with much what @grsmvg said). I see why we would want it though, but I think it would be better doing it in a v2 when things are more stable all over.

Besides I think such a re-write would take a lot of focus and energy away from bug-fixing, cross browser compatibility (as fore slate-react), and general UX concerns. And introduce a whole lot of new bugs in the re-write phase.

[renchap]

A point you did not mention: not using Immutable will also reduce the size of Slate's dependencies. If you are not already using Immutable in your app, it adds 55kb (minified, not gzipped) of JS to your bundles. It is really best for "core" libraries like Slate to come with as small as possible overall size (dependencies included).
Having less / smaller dependencies is also a big plus when you need to choose a library, for size but also licence concerns (as you need to check the licence for all deps), concerns about deps of deps becoming unmaintained or having security issues, …

[ppoulard]

IMHO, we can go further : I think that an entire refactoring of the internal Slate state would worth it if it includes CRDT considerations. I didn't dive inside that part of the code, and I may be wrong, but I'm not sure that 2 updates operations (add or delete) could be switched safely ; there is no notion of which user session did which change. We need this because undo/redo is tightly coupled to its owner (think about user experience : what if you concurrently edit things and when you want to undo you change, it would delete another user addition ?)
See #259

If you just replace Immutable with whatever, you'll have to replace later that whatever with a CRDT-friendly library if you intend to supply real-collaborative edition :

• someone has already proposed Teletype-CRDT : https://github.com/atom/teletype-crdt but it seems to work only on characters, not a JSON structure

• someone else proposed Automerge (https://github.com/automerge/automerge) but it seems that it works on JSON, not on characters (well, a chunk of character would be replaced entirely, not by delta ; but I'm not sure ; Automerge.Text support characters only). See Collaborative editing #259 (comment) in this example, Automerge have to escape from the Slate model (and the performance would be certainly bad), but if Slate was based on that model it would be much more efficient.
  It's worth to have a look at this presentation, because it defines the prerequisites :
  https://www.infoq.com/presentations/crdt-tachyon-collaborative-editing
  and from those principles, having an absolute reference of JSON + characters is what is needed ; in Slate, I found some data that looks like that :
  {"object":"operation","type":"remove_text","path":[0,1,0],"offset":0,"text":"Genetic","marks":[]}]
  (as far as I can understand / guess)

• Y.js (http://y-js.org/) that manages its own Map and List data structures

Maybe the latter is the more suitable.

[ericedem]

PRO: Custom records allow for methods/getters

Is this something we could just achieve with build in javascript classes (which support getters)? If so, could Immer support them?

[ppoulard]

In fact, to go on with my previous idea, Immutable (or Immer, that does the same but differently) just changes the state of a single client, whereas it doesn't make sense to manage immutable objects because an undo operation is no longer sequential when several people are editing collaboratively the same document.

We don't need to replace Immutable, we need a new strategy for having a data structure that accepts updates in orders that may vary according to each clients.

[ppoulard]

Ian suggested (#259 (comment)) that Slate should be unopinionated about OT/CRDT considerations.

I'm not sure it is easily possible : if Slate data model is aside CRDT data model, you have to maintain both together (like with the Automerge example), and as far as I know, the Slate data model doesn't consider absolute reference of its atoms, which is the key point in CRDT : the problem with indexes is that they shift, how would you find the location of a Slate change in the counterpart model ?

That said, I'm not well aware of the internal Slate data model (perhaps I'm wrong) ; as mentioned previously, it is hard to read because it relies on Immutable and I don't know that library.

[ianstormtaylor]

@grsmvg: But now that Immutable comes in a package deal with Slate, I find myself using quite some Immutable functions already, like value.document.nodes.get(-2) to get the second last node. This can also be achieved with (slightly more verbose) plain javascript or an imported utility library but it would definitely be quite a rewrite. I expect I'm not the only one who would need to rewrite quite some bit.

That's true, there are definitely some methods in Immutable.js that are useful. But it is definitely something that most people never find. Whereas if we using plain JavaScript data structures people would end up using lodash, or whatever else they liked, for all of these types of things. And as browser's further improve their standard library they could remove more and more of the imports.

@grsmvg: On one hand I'm wondering if this change is really needed on the short-term, since it will probably delay v1 by another few months, without really adding new functionality (rather removing it). But on the other hand, my fear of needing to rewrite quite a bit of code only supports making this change now, instead of in between v1 and v2, because then it might impact even more projects in production.

I understand where you're coming from here, but there is no timeline for 1.0 right now, on purpose. Because we want to be able to potentially make changes like this that are breaking (and sometimes tough) but greatly improve the library in the long run. Which is why I've kept Slate in "beta" and not made any promises on when 1.0 will land.

@skogsmaskin: In my opinion we need to focus on stabilising what we have now. There has already been two heavy refactors lately. Those were needed and gave very clear benefits right away. This one I'm not so sure about (I agree with much what @grsmvg said). I see why we would want it though, but I think it would be better doing it in a v2 when things are more stable all over.

Besides I think such a re-write would take a lot of focus and energy away from bug-fixing, cross browser compatibility (as fore slate-react), and general UX concerns. And introduce a whole lot of new bugs in the re-write phase.

This is fair, and I'm okay with waiting a little bit so that we can get the current 0.43/0.20 to be more stable. But if we decide to do it (and I'm leaning towards it more and more), it's not going to end up something we do in 2.0 because the whole point of being in "beta" is that we are able to make these changes now before the library settles and more and more people depend on its API's.

@renchap: A point you did not mention: not using Immutable will also reduce the size of Slate's dependencies. If you are not already using Immutable in your app, it adds 55kb (minified, not gzipped) of JS to your bundles. It is really best for "core" libraries like Slate to come with as small as possible overall size (dependencies included).

This is a great point! Not only by being a dependency, but the extra bloat it adds to defining records would also reduce some of the core Slate bundle size too.

@ericedem: Is this something we could just achieve with build in javascript classes (which support getters)? If so, could Immer support them?

This is something I leaned towards at first, when we were focused mainly on Immer as the main question. But as it evolved, I realized that most of the benefits in terms of performance and simplicity actually come not from Immer, but from being able to use plain JavaScript objects to avoid parse/serialization steps, and to avoid having two different representations for objects.

At this point I'm more interested in the plain data structures than Immer itself. If there were some (unknown) better library that Immer, I'd use it while keeping the plain data structures.

---

@ppoulard I'm interested in Slate being able to support CRDT, however I have yet to see a CRDT approach that I think feels viable for nested documents and for real-world use cases where documents don't balloon to huge sizes. For that reason most of Slate's collaborative support has been focused on making OT possible first, which it already is today with the current codebase. (And it still would be with plain data structures.)

Since CRDT and collaboration is pretty convoluted, and seems so far like something that many people want, but few people actually have the time to do, or don't want to put in the effort, it's not something that I'm going to be personally adding to core. And for that reason I don't want it to hamper any decision we can make here.

If it turns out to absolutely be the best way, and it turns out also that we absolutely have to use non-plain data structures, we can cross that bridge in the future when someone puts in the work to add CRDT. (But it might be very far in the future.)

---

Thank you everyone for your responses! (And feel free to provide more if you think of other things too.) The more I think about this, the more it seems like something that would be very beneficial to Slate. The tougher part is that decoupling from Immutable.js would be harder in cases where its custom (helpful) methods are used. That's the biggest blocker in terms of a refactor.

I'm okay with waiting a little bit on this now, to let the current version of slate and slate-react stabilize from the recent two refactors. That way we have a more sound set of versions that people can wait on if they can't refactor out Immutable.js right away, so we don't leave them stranded. But this isn't something that I think we'll be waiting for 2.0 or 3.0 to do in years—that's why Slate is in beta.

Thank you!

[ianstormtaylor]

Thinking about it more, I think the best way to make this change would be to do it in two stages:

1. Deprecate Methods. Change all of the current instance methods to be implemented as static methods, and created deprecation warnings for the existing instance methods. This would be completely backwards compatible, with warnings logged for all changes. But it also continues to use Immutable.js in the interim step.

2. Remove Immutable.js. Change the models to not use Immutable.js any more. This would not be backwards compatible, because of Immutable.js's own helper methods that it exposes, which would be the hardest thing to migrate properly. However the instance-to-functional migration would already have happened with the deprecations previously.

[renchap]

If you go with those big changes, might I suggest starting to use Typescript for Slate's core? It is not hard to incrementally use typescript nowadays as its only a Babel plugin (I did a coffee => js => ts migration for my app incrementally, I dont regret it at all and both can really cohabit).

[PierBover]

But it is definitely something that most people never find. Whereas if we using plain JavaScript data structures people would end up using lodash, or whatever else they liked, for all of these types of things. And as browser's further improve their standard library they could remove more and more of the imports.

The JS community moves very fast and many projects have suffered from coupling to solutions that died or simply went out of fashion (eg: Coffee script). By using native structures Slate will be able to better stand the pass of time.

[ianstormtaylor]

@renchap I'm open to using TypeScript, but I'm not sure I want to couple it to this idea. But if someone wants to help TypeScript adoption, I think a decent next step would be to open a pull request with a single small-to-medium-sized file converted to using types and we can have a discussion around it there.

[renchap]

@ianstormtaylor The thing with moving to Typescript is to start to type your core model first. Once it is typed, you can start to type the functions that are using it, and expand. This allows you to avoid putting a lot of any types when converting.
Hence why I think it might go along, as if all the structure is rewritten using a new paradigm, it is a good idea to rewrite it in TS first.
But I agree this is not 100% tied, and it will be probably best to have a TS-compatible setup (Babel, linter, …) first, so the impact of moving to Typescript for the model change is just changing the file extension and starting to use types.

I will try to find time to open an experimental PR with this.

[zhujinxuan]

Hi, may I know how we can deal with cache with immer?

I am afraid of something like this in user side:

const path = document.getPath(path);
path[0] = 1;

[ianstormtaylor]

@zhujinxuan that's a good question, I'm open to ideas.

Thanks to Immer, I believe all of the core JSON models would be frozen in development mode, so they can't be mutated. We might want to do a similar freezing for return values.

For caching, one thought was that potentially the only cache that truly matters is the keys -> paths dictionary. The others might not be necessary with the move away from Immutable.js since the read performance would be improved.

For the keys -> paths, I was thinking about whether we could have a global cache that was restricted heavily in length, maybe the last ~100 lookups by reference? It might get us enough of the benefits, since the caches often become stale. And we could potentially limit it to only being used on Document nodes too for further guarantees.

But I'd love to hear other ideas here!

[zhujinxuan]

@ianstormtaylor BTW, there are two caches. The other is this.__cache.validateNode(to skip normalized node in normalization), though it does not matter the complexity.

I think it is a good idea of global keys->path, but in this solution, it seems we cannot simply Block.fromJSON({...}) because Block.getDescendant will fail because the globally keys->path is not set for this Block (if the path is relative to the document root).

I am not sure whether it is a good idea to have an global WeakMap(node, {key, path} ); Because it is a weakmap, we do not need to worry about the memory leak if the node is recycled. And we can easily visit other nodes' cache without using node.__cache.

[ianstormtaylor]

@zhujinxuan that's a good point, it could not be Document-only in that case, you're right. Using a WeakMap could be interesting too!

[zhujinxuan]

@ianstormtaylor Great. I will open a experimental PR to replace the current __cache with a global weakmap. And we can see how the code may be alike.

[ianstormtaylor]

Just came across another place where we are weirdly using the two different representations... the decorateNode middleware returns decorations, but its the one place where you can't just return either without downsides.

Right now if you return Array, all the other decorateNode middleware must also use Array. And if you return List, then all the others would need to use lists. Or they would all need to handle both cases. (This is required to get next() to work as expected.) For now everywhere it using arrays, and it's not big issue, but it's just another place for confusion.

[steida]

@ianstormtaylor That's why types are so helpful. Can't imagine writing code without them anymore. For newcomers, I highly recommend learning algebraic polymorphic types. It's really a game changer. First in Flow, but TypeScript supports them as well https://www.typescriptlang.org/docs/handbook/advanced-types.html

It's also useful to know https://flow.org/en/docs/lang/nominal-structural/ types. Because Slate can be written as classic OOP class & inheritance "hell" or as plain data with algebraic types (which really rocks).

Until you will feel safe when thinking about that, I would not recommend using any typed language abstraction over JS.

Example https://gist.github.com/hallettj/281e4080c7f4eaf98317#file-tree-no_classes-js-L3

I am afraid you will have a desire to rewrite everything once you will switch to types. Because types work the best when everything is typed.

[steida]

Btw, asked Lee Byron https://twitter.com/steida/status/1058442668911529984 what he thinks about immutable to Immer, and he suggested Immutable 4.

[zhujinxuan]

Hi, I think @thesunny 's idea on using a $ as an interface is a good solution, but it may have a small problem when:

$.getActiveMarks(value).find(m => m.type === 'bold')
$.getActiveMarks(value).find(m => m.type === 'bold').anchor
$.getActiveMarks(value).find(m => m.type === 'bold').doSomething()

Then what we shall return for $.getActiveMarks(value).find(m => m.type === 'bold'), a bold mark itself so we can continue the chain call, or a bold mark proxy that we can continue the chain call.

We can allow the bold mark proxy visit mark properties just like mark, but it will require us define proxies for our current models for chain call.

[Nantris]

Can anyone comment on what kind of difference this would make in node rendering speed in terms of percentage improvement? I pretty much never see performance issues with Slate itself but rendering a document for the first time can have some slight delays. The slowdowns are a lot more significant on mobile but pretty much imperceptible on desktop.

[thesunny]

@zhujinxuan yeah, my proposal is syntactic sugar primarily designed to make things a little easier to read while still respecting the idea that all the values are pure JSON. It's not a very good replacement for models with methods.

The equivalent syntax would be something like:

$.getActiveMarks(value).find(m => m.type === 'bold')
$.getActiveMarks(value).find(m => m.type === 'bold').anchor
const marks = $.getActiveMarks(value).find(m => m.type === 'bold')
$.doSomething(marks)

Another proposal could be to wrap the JSON before calling the methods and I actually kind of like the readability of this. I was going to propose this as well originally:

$(value).getActiveMarks().find(m => m.type === 'bold')
$(value).getActiveMarks().find(m => m.type === 'bold').anchor

// option 1
const marks = $(value).getActiveMarks.find(m => m.type === 'bold')
$(marks).doSomething()

// option 2
$($(value).getActiveMarks.find(m => m.type === 'bold')).doSomething()

The second syntax is straightforward to implement which is nice:

class Node {
  getActiveMarks() {
    // ...
  }
}

class Document extends Node {

}

const CLASS_LOOKUP = {
  node: Node,
  document: Document,
}

$ = function (value) {
  const TheClass = CLASS_LOOKUP[value.type]
  return new TheClass(value)
}

[thesunny]

Hmm... and yet a third way is to always return the values wrapped automatically and have the data always kept in a JSON value. This way it's easy to convert back and forth without any work.

In other words:

$(node).json === node

Or to get active marks, do something with it, and then get the resultant JSON value:

$(value).getActiveMarks().find(m => m.type === 'bold').doSomething().json

[thesunny]

It feels like I'm having a discussion with myself but I have been thinking about the alternatives and the trade-offs.

The three alternatives are:

• methods off of types like Node.getActiveMarks
• methods off of generic like $ but really can be anything including a regular name
• an object constructor off of generic like $ but really can be anything including a regular name

I'm gravitating towards the $ with method because it is safer from upgrades and it is safer from user errors in this style $.getActiveMarks(node):

• If we have to use the type before the method name, the problem is if future versions of Slate required us to change the code for certain types. For example, let's say that we have a method like Node.getText but we later find due to an internal implementation change that Document.getText or TextNode.getText need to have a different implementation. This also makes the code harder to use because we have to know the right version to call. $.getText(node) is easier to use and survives the implementation change without having to refactor user code.
• For the proposal where we wrap objects before calling methods and then returning another wrapped object, we have the dangerous issue of forgetting to return the json object. If we use the wrapped json object, the editor might not crash but we're putting in bad data that eventually may cause issue and it would be hard to detect where the wrapped object came from.
• Even if we used the version where it always returns a json object after the call, we end up having references to wrapped objects like node = $(myNode) and it could be easy to accidentally pass the wrapped node into the editor. Feels safer to never have any wrapped objects lying around.

[Nantris]

@thesunny - I've been following the discussion; I just don't feel that I'm in the position to make good design choices in this situation. Keep up the good work!

[dmitrizzle]

Same here, following all Slate discussions but due to time and experience constrains can't always contribute. If it helps, my personal use with Slate Editor, requires better performance on large documents; bundle size is a non-issue.

My Slate package is way out of date so I don't mind fixing incompatibilities either, as long as they aren't too crazy. I'd imagine this switch would be the largest breaking change before v1.0.0.

[zhujinxuan]

@thesunny I am thinking whether we can use a proxy interface for that. For example

p(n => n.getActiveMarks().find(m => m.type === 'bold').doSomething())(node)

shall return a pure node.

However, I am feeling adding a new proxy on top of immer proxy only make things more complex.

[Nantris]

@zhujinxuan can you expand on your comment? Can you put the equivalent code (either current or as proposed by thesunny) for what you're proposing side by side? I'm not clear on what currying node does in this context.

[ianstormtaylor]

I think using just Slate.* would be fine. I'd want to avoid $ since it's so tightly coupled in peoples's minds to jQuery. But I agree with you @thesunny that they should be methods that return plain JSON objects, not any custom chaining logic.

[thesunny]

I like Slate.* as well and was a little worried about conflating the behavior with jQuery. Anybody who likes the syntax of $ can always assign it as well const $ = Slate.

[brendancarney]

Could we start by mimicking most of the API with functions and getters on the object directly?

Fake example API:

const value = {
  nodes: [
    { key: 1, text: "Hello " },
    { key: 2, text: "World" }
  ],
  get text() {
    return this.nodes.map((node) => node.text).join("")
  },
  getTextForNode(key) {
    return this.nodes.find((node) => node.key == key).text
  }
}

value.text == "Hello World"
value.getTextForNode(1) == "Hello "

[dragosbulugean]

@ianstormtaylor what's your current position on this matter? has anybody started working on this migration?

[ianstormtaylor]

@dragosbulugean good question. For the next steps on making this happen, check out: #2495 — a list of the smaller steps that go into being able to make this change. If anyone can help with any of the steps (or happens to discover another thing we need to fix) that would be awesome!

As soon as we have the precursors done we can make the breaking change.

[wmertens]

Question: What is the benefit immer would bring over plain objects? It's easy to freeze all incoming values (in dev mode*) but otherwise just use them without changing them or adding proxies.

I'm just playing devil's advocate - it could very well be that deep changes are too annoying to to manually, or that immer does them lazily which might make things faster than plain objects.

Another potential problem is that immer seems to use Proxy which is not available in older JS engines (mostly IE and older Samsung Internet, so it might not be an issue at all)

* freezing might incur a performance penalty, at least in 2017

[ianstormtaylor]

I believe immer would be an implementation detail, only inside Slate core. To everyone else it’s just a regular JSON object that happens to be frozen to imitate immutability.

I’m unsure of what the browser support is. But we don’t want to re-invent the wheel and write the immutability logic ourselves when a popular existing solution with no external API impact exists.

[knpwrs]

Question: What is the benefit immer would bring over plain objects

Directly mutating objects is much more natural than immutably updating objects. This is presumedly why Immutable.js was chosen in the first place, because it offers an API which feels like direct mutation.

Another potential problem is that immer seems to use Proxy which is not available in older JS engines (mostly IE and older Samsung Internet, so it might not be an issue at all)

Immer automatically falls back to a getter/setter-based solution, much like Vue or Mobx. Source code here: https://github.com/mweststrate/immer/blob/0ad9a91e189685b24714686bd6a42699ce02feae/src/es5.js

[renchap]

Question: What is the benefit immer would bring over plain objects? It's easy to freeze all incoming values (in dev mode*) but otherwise just use them without changing them or adding proxies.

Immer already freezes objects only in dev mode

I'm just playing devil's advocate - it could very well be that deep changes are too annoying to to manually, or that immer does them lazily which might make things faster than plain objects.

I feel that properly dealing with deep updates would end up with rewriting Immer

[wmertens]

I feel that properly dealing with deep updates would end up with rewriting Immer

I had a look at Immer, and I think I agree. The main benefit of Immer would be doing multiple mutations in one go, which would either not be implemented or recreated when doing immutability manually.

The only way to really be sure is to implement it both ways and benchmark, which I think would show trade-offs on both sides.

I resign as devil's advocate ;)

[knpwrs]

The only way to really be sure is to implement it both ways and benchmark, which I think would show trade-offs on both sides.

I've actually done benchmarks of immer vs ramda vs plain JS for deep immutable updates. I don't have the benchmarks around anymore, but all solutions were on the order of tens of thousands of updates per second and trading blows for the top spot. In terms of performance, there was no clear winner.

In terms of simplicity, however, there was a clear winner. I think it's obvious by now which one I like. 😄

[lazharichir]

Could we start by mimicking most of the API with functions and getters on the object directly?

We should definitely limit the breaking changes as much as possible for the end users. Using JS Class and the get getters would help with keeping the API similar, if not the same. Especially as the API is actually super well designed and makes so much sense while using it.

However, I do agree ImmutableJS is a pain, especially server-side. Have to give us on using the Value.fromJSON() because of that and now I work just with the JSON tree.

[knpwrs]

Immer supports classes now, FYI: https://github.com/mweststrate/immer#supported-object-types.

There is also a low-level createDraft/finishDraft api that is very useful for building libraries: https://github.com/mweststrate/immer#createdraft-and-finishdraft

[ianstormtaylor]

I forgot, one of the issues in terms of removing keys from nodes, is that when rendering in React-land there's no stable key to use, which leads to issues.

[ianstormtaylor]

Another thought: I think once we’re using JSON-based models we’ll move fully to interface-based helper functions.

Right now things we have “mixins” that approximate this by attaching common methods to the prototypes. And we can rely on instances to do instance.object === ....

But in the future we’ll be exposing a bag of helper functions that operate on plain objects and arrays. We could continue to require that those objects have an .object property to tell us what’s what. But that’s kind of awkward. Instead we can just operate against an interface.

Since decorations and annotations both implement the Range interface, you could call Range.contains(annotation, path). And since they (now) both implement the Mark interface too you could use them with mark-related helpers.

[ianstormtaylor]

Fixed by #3093.