search

ianstormtaylor / slate

A completely customizable framework for building rich text editors. (Currently in beta.)
http://slatejs.org
MIT License
29.79k stars 3.24k forks source link

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

Closed ianstormtaylor closed 4 years ago

ianstormtaylor commented 5 years ago

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:

However, it does have some benefits:

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.

https://github.com/ianstormtaylor/slate/blob/8eb8e26958ef6c9337e838bfd46fc2cc55d7fe51/examples/rich-text/index.js#L54

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:

https://github.com/ianstormtaylor/slate/blob/8eb8e26958ef6c9337e838bfd46fc2cc55d7fe51/examples/rich-text/index.js#L66

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.

https://github.com/ianstormtaylor/slate/blob/8eb8e26958ef6c9337e838bfd46fc2cc55d7fe51/examples/rich-text/index.js#L148

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:

https://github.com/ianstormtaylor/slate/blob/8eb8e26958ef6c9337e838bfd46fc2cc55d7fe51/examples/rich-text/index.js#L293-L295

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:

https://github.com/ianstormtaylor/slate/blob/8eb8e26958ef6c9337e838bfd46fc2cc55d7fe51/examples/rich-text/index.js#L41-L43

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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 https://github.com/ianstormtaylor/slate/issues/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 :

Maybe the latter is the more suitable.

ericedem commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

Ian suggested (https://github.com/ianstormtaylor/slate/issues/259#issuecomment-242504922) 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 commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

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

ianstormtaylor commented 5 years ago

Thanks @steida, that's helpful. I think Immutable.js 4 would solve some of the performanced-related read CON, but it doesn't seem like it really addresses the others? I know that it improves read performance for Record objects, but not sure what else it does? Last time I tried to upgrade us to it it broke the tests so I put it off.

As for TypeScript, that makes sense that it trends towards being an all-or-nothing decision. For context, we actually already have docblock-style comments that include typing information for lots of the codebase—often using algebraic types. (But they are often incomplete or outdated because by nature of being comments.)

https://github.com/ianstormtaylor/slate/blob/24af6113dc3372fcd8779039b599f4175670d4ae/packages/slate/src/interfaces/element.js#L222-L242

So I'm not that worried about that, more excited. There are many concepts that we already re-use in places as if we were forced to by typings purely out of desire for consistency.

The things I'm worried about around much more around process and maintenance, and to get more concrete answers to the tangential pieces (eg. updated build steps, new dependency management needs, updated test writing patterns, typings API naming patterns, etc.) that people often gloss over when considering the tradeoffs. But let's talk about all of that in #560 instead of here, since I want to keep these two decisions separate if possible.

steida commented 5 years ago

Sorry for maybe a dumb question but related to potential model rewrite. Isn't possible for all commands like toggleMark etc. being pure functions aka with value input and output? I am playing with useReducer hook and it would be nice to be able to control Slate model within reducer.

ianstormtaylor commented 5 years ago

@steida can you give a code sample? A few constraints to remember:

But it could be, not sure.

steida commented 5 years ago

operations as the lowest-level changes for collaboration support

I am not sure I understand. Functions compose well. What cannot be a transformation? Are you familiar with the concept of pure functions composition or I missed the point?

allow commands to be overridable by plugins

Why they should be overrideable? Immutability ftw. There should be no generic command I suppose. A plugin is just another transformation aka function composed of other functions.

Need to be able to run queries inside commands.

It sounds like a command query separation violation.

Sorry if my comment does not make a sense. I will try to explain it better tomorrow.

steida commented 5 years ago

Code sample:

function editorReducer(value: SlateValue, action: EditorAction) {
  switch (action.type) {
    case 'toggleMark': {
      return toggleMark(action.mark, value);
      break;
    }
    default:
      return value;
  }
}
ianstormtaylor commented 5 years ago

@steida let's talk about this in https://github.com/ianstormtaylor/slate/issues/2342 instead of here to keep it focused. But for other context as to why some of those constraints are things we want, check out https://github.com/ianstormtaylor/slate/issues/259 https://github.com/ianstormtaylor/slate/issues/2334 https://github.com/ianstormtaylor/slate/issues/2066 https://github.com/ianstormtaylor/slate/issues/2206 https://github.com/ianstormtaylor/slate/issues/2249 for current thinking. Not saying it's impossible, or even that those constraints are hard coded, but those are the goals we're aiming for.

steida commented 5 years ago

So, how did you decide? Will you rewrite it? Or make it work, make it right, make it fast?

PierBover commented 5 years ago

Also, is there any kind of ETA for this deep change? Like mid or late 2019?

I'm building a commercial product with Slate and I would need to know approximately how long the project will be stalled since this change will impact the API.

ianstormtaylor commented 5 years ago

So, how did you decide? Will you rewrite it? Or make it work, make it right, make it fast?

@steida what does that even mean?

Also, is there any kind of ETA for this deep change? Like mid or late 2019?

I'm building a commercial product with Slate and I would need to know approximately how long the project will be stalled since this change will impact the API.

@PierBover Slate's in beta right now, so these kinds of questions are not easy to answer. We are not at the stage of this project's lifecycle that we can give development roadmap guarantees many months or years into the future. But if not knowing this change's timeline would be prevent you from using Slate, then that might mean that you shouldn't be using it in production.

If by "stalled" you mean the time from when this change starts development to when it is merged, I'd estimate from a few days to a few weeks. Most changes so far have had short start-to-merge times. (For example, see https://github.com/ianstormtaylor/slate/pull/2337 https://github.com/ianstormtaylor/slate/pull/2221 https://github.com/ianstormtaylor/slate/pull/1313 https://github.com/ianstormtaylor/slate/pull/1993.)

Based on all of the arguments PRO and CON above, I think it is likely that we will make this change, but I don't have a timeline. It could be in a week, or it could be in a few months, or it could be in a year.

Slate is not sponsored by any organization and is run entirely on individual contributions. When I need a feature added, or a bug fixed, or part of the architecture improved for my own use then I devote time to it, and same goes for everyone else. And for the most part, if people need something but no one needs it enough to devote time to it, it doesn't get done.

PierBover commented 5 years ago

Thanks, just wanted to get an idea. I completely understand your position.

steida commented 5 years ago

"Make It Work Make It Right Make It Fast" This formulation of this statement has been attributed to KentBeck; it has existed as part of the UnixWay for a long time. http://wiki.c2.com/?MakeItWorkMakeItRightMakeItFast

It means, by my own personal experience, I would first complete Slate before rewrite it from scratch both with types and different state model. Or, I would stop everything else and rewrite it entirelly from the scratch. But if you have another opinion, feel free to ignore me.

Dundercover commented 5 years ago

If we are mostly just interested in the js objects of immer then maybe it's better to roll our own freezing utility for dev environments and make sure we have methods for everything so we can "unfreeze" to mutate (like in #2030).

Edit: or maybe immer doesn't suffer from any performance hit with its draft concept in production.

klis87 commented 5 years ago

@Dundercover According to my understanding actually Immer would increase performance, according to benchmarks with similar gains to ImmutableJS, but without cost of converting to and from Immutable objects to native ones.

Dundercover commented 5 years ago

@klis87 Alright, I don't understand how it could be though, since it's only about mutating a state in production. But it might be good to use immer anyways if we don't want to deal with the complexity/maintainability.

klis87 commented 5 years ago

@Dundercover mutating object is faster than immutable updating in JS, because this requires creation of copies, which is especially problematic for deeply nested objects. Not only copying takes time, but also it pollutes memory and gives more work to garbage collector. Solutions like immer/immutable js is not only about protection from unintended mutations, but also/mostly for improving performance.

Dundercover commented 5 years ago

@klis87 that is true, I didn't think about that we still needed immutable objects in production.

ianstormtaylor commented 5 years ago

Another thing to consider is how to handle keys for nodes. Because right now they are auto-generated for nodes when constructed. But if we want to eliminate the traverse-and-construct step of things, we won't have that opportunity to create keys.

We might need to store temporary keys in a WeakMap somehow as well, but this would change things to result in a new key each time the node is changed. Potentially we might want to introduce a new idea of "path reference" instead, which can be tracked across operations? The real reason we need keys on the frontend is to keep track of nodes even as operations are applied. But if we let people create a "ref" to a path that could resolve that need.

It should also allow people who want to set long-lived keys set them directly on the objects without having to jump through hoops.

ianstormtaylor commented 5 years ago

Another thing to consider is how keys are used in slate-react to find nodes in the document. We might need to move to react-specific data attributes that are attached to the DOM, instead of re-using keys for this purpose.

lxcid commented 5 years ago

I think the most important property in Immutable.js that I value is immutability. Immutability have the benefit of triple equals (===) the record or sub-record and if its true, you are guarantee that nothing is changed. This work really nicely with shouldComponentUpdate() or pure component.

Also, because its have structural sharing, this is achieve with minimal cost, as oppose to actual copy and mutate.

The actual naming for these properties are Persistent Data Structure for anyone interested.

I believe Immer could achieve Persistent Data Structure, without an over bloated interface.

My worry is that contributors or users coming in will introduce mutation to plain object and break this property which can be hard to debug if Slate depends on it.

knpwrs commented 5 years ago

My worry is that contributors or users coming in will introduce mutation to plain object and break this property which can be hard to debug if Slate depends on it.

While this repository is written in JavaScript, I think for something like this it may be worth considering TypeScript. A while back I contributed changes to immer which let it modify readonly properties: https://github.com/mweststrate/immer/pull/161

Here's a very basic example from that PR:

import produce from 'immer';

interface State {
  readonly x: number;
}

// `x` cannot be modified here
const state: State = {
  x: 0;
};

const newState = produce<State>((draft) => {
  // `x` can be modified here
  draft.x++;
});

// `newState.x` cannot be modified here

I'd say if there's serious consideration to move on to using immer instead Immutable.js then you might as well simultaneously rewrite at least the data layer in TypeScript since the conversion from Immutable.js to immer constitutes a rewrite anyway. Anything representing immutable state can then use readonly modifiers or the Readonly mapped type.

thesunny commented 5 years ago

@ianstormtaylor

Coming into this discussion late but after reading this thread I thought about a possible solution to the "Pure JavaScript objects work better" but "Pure JavaScript objects don't have custom methods" issues.

Quite possible I'm missing something but how about a syntax like this using the examples in the first post?

$.getActiveMarks(value).some(mark => mark.type === type)
$.blocks(value).some(node => node.type === type)
$.getParent(document, blocks[0].key)
const isType = $.blocks(value).some(block => {
  return $.getClosest(document, block.key, parent => parent.type === type)
})

For it to work, each of the values would have to have a type associated with them but I think the current model already does.

The implementation would look something like

// defining methods for $ with how something like inheritance might work
const node = {
  text() {},
  filterDescendants() {}
}

const value = {
  ...node, // inherit from node
  getActiveMarks() {},
  blocks() {},
}

const document = {
  ...value, // inherit from value
  getParent() {},
}

// This is the object you use to call everything else defined where the key is the object's `type`
const $ = define({ node, value, document })

function define(definitions) {
  const methodNames = new Set()
  Object.values(definitions).forEach(definition =>
    methodNames.add(Object.keys(definition))
  )
  const $ = {}
  methodNames.forEach(methodName => {
    $[methodName] = defineMethod(methodName)
  })

  // defines the method on the $ object
  function defineMethod(methodName) {
    return function(target, ...args) {
      const method = definitions[target.type][methodName]
      if (method == null) throw new Error(`method ${methodName} not found for object type ${target.type}`)
      method(...args)
    }
  }
}
zhujinxuan commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

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:

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):