ianstormtaylor / slate

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

mismatch between "editor" and "value" concepts #2206

Closed ianstormtaylor closed 6 years ago

ianstormtaylor commented 6 years ago

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

Discussion.

What's the current behavior?

Just opening this up for discussion, since it's something that I think Slate hasn't really solved well so far, and leads to it being hard to decide in either direction for the API...

Right now there are two concepts vying for the "top spot" in the architecture:

  1. value — which holds the document, the selection, the history and the schema. This can easily be created server-side (or wherever), since it's not coupled to the view layer at all. (I'd consider change to be equivalent to value here, since they hold all the same data.)

  2. editor — which holds the plugins, the current value, and the schema to an extent. This is less easy to use server-side, since the editor is inherently tied to the view layer. (Although this need not always be the case.)

So far, it's been unclear which is the "primary" concept. For example...

What's the expected behavior?

I'm not totally sure. This is something that I've wrestled with, but never found a nice mental model for being clear about the distinctions between the two. I'd love for anyone with ideas to comment! (Even if you aren't sure yourself, anything is helpful!)

I think it's helpful to have a few goals:

zhujinxuan commented 6 years ago

but sometimes, for example if you want to trigger an async change later, you need to use the editor.

Perhaps we shall expose editor.change as get change instead of change(fn), then it appears reasonable that editor.change is same with change in onEvent(chang, editor), but different in the setTimeout. (But the problem is how to flush the change in setTimeout? calling editor.onChnage(editor.change) is not a good idea.)

Perhaps we shall expose editor.requestNextCycle to deal with the asynchronous calling? We cannot guarantee that window.requestAnimation is exactly after this lifecycle update if multi-components are updated in the same time with editor.value.

Right now there's a confusion in that the editor's plugins create a final schema object that gets set on the value.

Can I know where we need about value.schema? It seems that we run normalization by insidestack.run rather than with value.schema. Perhaps we can have stack inside change to run server side tests?

ericedem commented 6 years ago

Hmm I've been wrestling with this one myself. The mental model I have for the Editor is that it is simply the view layer. Given a bunch of props like value / schema etc., Render something to the page. The value is then just the pure state of the app: what is the current snapshot of the app's state.

I think the problem is that this gives us the "view" and the "model" but it doesn't really tell us where the controller lives (in a traditional MVC architecture). So the current "controller" is a combination of change, value and the editor that don't always know how to talk to each other. This is why there is some confusion about whether to make changes you need to talk to the value or the editor.

I wonder if there should be a third component like Controller that contains all the event / state management. Then the "editor" is just a simple view layer that renders and proxies interaction events into the controller. The controller would also just be pure non-react JS, easy to work with on the server side. It would probably also be easier to test than the editor.

The big downside I see to this is that in React a component is generally also the controller. This might cause us to move further away from React best practices.

zhujinxuan commented 6 years ago

I think one problem is that we expose editor as React.Component, but its interface appears like something/proxy from packages/slate.

bryanph commented 6 years ago

I like @ericedem's suggestion. Perhaps we should consider Editor as a target environment and allow plugins to target different environments. This would also make it easier to target other environments later like react-native for plugins or even completely different frameworks than React (if someone would want to do this). Just a late night thought :man_shrugging:

ianstormtaylor commented 6 years ago

@bryanph can you give some pseudocode to explain what you mean? or explain more?

justinweiss commented 6 years ago

I don't really have anything concrete to add, other than that I think of Editor purely as the view/controller layer, and that I really really like being able to work with a Value outside of the context of showing an editor. It's one of my favorite things about Slate -- with just a Value, I can create and apply operations, I can apply schemas, I can serialize / deserialize, etc. I get all the transformations I want, and just get a new Value at the end.

A few things people have mentioned here that I'd appreciate:

ianstormtaylor commented 6 years ago

@ericedem I agree with you, having a plain-JS "controller" concept is compelling. It would help the server-side story, it would probably clean up some of the existing client-side story, and it would make it more clear where the interoperability boundary is for plugins and other view layers in the future.

@justinweiss I agree with you too, I'd like history to be more pluggable. And it sounds like you're talking about similar things to @ericedem as well.


I think one thing I'm struggling with is where the plugins and schema live. Right now they sort of live in both places. And there are tradeoffs in both directions:

  1. If they live in the controller, then you wouldn't be able to call plugin-defined "commands" if you only have the value model instance. You'd need an instance of the controller as well.

  2. If they live in the value (model), it feels slightly weird to have the model's API be something that is fluid and defined dynamically in plugins. It's possible, but it would need to either use string-based command names (reinventing methods), or use Proxy to make it seamless.

If we approach it from the plain-JS controller mindset, then 1. might not be as much of an issue, because instantiating a lightweight Controller isn't hard. And this does feel the most natural to me.

We could decide to get rid of the:

value.change()...

In favor of only having the:

controller.change(change => ...)

But that loses some of the niceties that @justinweiss is talking about. But @justinweiss I'd be curious to hear which of those are truly most important to you. I'm not sure I'd personally mind it if the schema was tied to the editor instead of the value. But I would definitely miss the nice serialization of values. Doing this would result in maintaining:

And losing:

This would make it clear that these things are the domain of the controller:

controller.change(change => ...)
controller.command('toggle_bold')
controller.query('is_bold')

What are your thoughts on that? (Anyone!)

justinweiss commented 6 years ago

Thinking about it more, Schema isn't really a property of a Value, it's a property of how you use it. If we had a Controller, it seems like the right place for those.

It shouldn't be much of a problem -- If I could, in a Node app (for example), manually instantiate a controller, hand it a Value.fromJSON and a schema, run changes on it, and pull either an updated Value or a bunch of Operations back out of it, that wouldn't be much more complicated than it is today. And if we can still hand a Value a list of Operations and get a new Value out of them, there's always an out if going through the Controller isn't feasible. You just wouldn't be able to guarantee those operations would put the Value into a valid state. But if the list of operations originally came from change methods, you could assume that normalizing operations were already in that list.

ianstormtaylor commented 6 years ago

@justinweiss nice, I agree! In the OT server use case where you’re receiving the low-level “valid” operations you can operate on the value directly. And for other use cases you can instantiate a thin controller.

I think it might also make it possible for us to combine the idea of the “simulator” and the “controller” potentially which would be nice for testing.

justinweiss commented 6 years ago

@ianstormtaylor Yeah. In the OT case, though, you could have valid simultaneous operations put the Value into an invalid state.

Imagine you have two empty paragraphs, client 1 deletes paragraph 1, and client 2 simultaneously deletes paragraph 2. Those are both valid operations on the document, but you'd end up with a Document with no nodes.

So you'd probably want the Schema / Controller on that side, too, you just wouldn't need an Editor.

ianstormtaylor commented 6 years ago

I'm just going to leave a summary of my current API architecture thoughts here...


At the end you end up with a clearer delineations:

Layer Concepts
Model Value, Operations, Nodes, Marks, Ranges, Data
Controller Editor, Changes, Schema, Commands, Queries, History
View <Editor>, <Island>, render*

The only overlap remaining that I can see still is that "plugins" end up defining logic that lives in both the Controller and the View layers. But I'm not sure if there's a way around this. (If anyone has ideas I'm open!)

How does that sound?

ericedem commented 6 years ago

Hey this sounds great, just a couple questions:

Add "queries" support.

Can you clarify what these queries would be used for? Is this for grabbing a subset of the value? Would this replace access like Editor.value?

Also, I'm trying to think through what the interface to react would look like. Would the user create an instance of Editor and pass it into the react editor? Or would the user still pass in through a similar interface to what they are doing today, and have the Component build out the Editor Controller?

Is the React component still going to be controlled like it is now? By emitting the value in a change event and passing it back in through props? Or would the React component emit the entire EditorController on every change?

ianstormtaylor commented 6 years ago

@ericedem great questions! Thanks for reading and digging in.

Can you clarify what these queries would be used for? Is this for grabbing a subset of the value? Would this replace access like Editor.value?

Sorry, this is from #2066. Basically once you add "commands" you start to realize that many things that work with commands need to ask the editors questions to know whether to perform certain logic, and you get a parallel "queries" feature for doing that. If "commands" are schema-specific functions that perform changes. Then "queries" are schema-specific functions that return information.

Also, I'm trying to think through what the interface to react would look like. Would the user create an instance of Editor and pass it into the react editor? Or would the user still pass in through a similar interface to what they are doing today, and have the Component build out the Editor Controller?

Is the React component still going to be controlled like it is now? By emitting the value in a change event and passing it back in through props? Or would the React component emit the entire EditorController on every change?

I'm sketching this out right now. I'd like to keep the component interface the same, since it feels pretty nice to me, and feels React-ish. Such that the <Editor> component is actually just going to create its own Editor controller under the covers, and rely on the controller to handle some of the logic for it.

However, that underlying controller is never publicly exposed to the user. So for example, the <Editor> will wrap the Editor controller, and when creating a Change it will have a change.editor pointing to the component instance. (The component will be in charge of mimicking the Editor interface so that they are interchangeable.)

By doing that, the <Editor> remains a "ViewController" in the React sense. I think the pattern that will end up shaking out is that each view layer can adopt their own patterns. For React, it merges the "view" and "controller", and thus it makes sense to do that here. If some other view layer had two separate concepts, they could separate them.

However, on the server-side you'd just use the Editor directly, with the same API (since <Editor> mimics it).

What this means is that some plugins, if they happen to rely on React-specific properties of the change.editor will be React specific. But this is already the case with event being synthetic events. And I think this is inevitable so we should embrace it.

How does that sound?

bryanph commented 6 years ago

@ianstormtaylor So what would be possible with this architecture that isn't possible currently? I'm not sure I fully understand the issue.

Some thoughts:

As for what I meant with targeting an environment, this was a thought I had because plugins to me seem to do two things:

Since plugins tend to wrap functional behaviour, like a nested list or adding an image block, I thought perhaps the environment-specific code like adding event listeners could be specified in the plugin as belonging to a certain environment, like React or React native. So that when iterating through all the plugins you would potentially get support for multiple environments depending on which environments the plugin supports. However, thinking about it further, you'd probably want to keep these plugins separate, so have a slate-react-plugin-list and a slate-react-native-plugin-list, with some duplicate code between them for extending the schema, normalization, etc.

But if we want to support React Native in the future we'd probably want to start naming plugins to reflect that they only work in React and not in React Native

ianstormtaylor commented 6 years ago

@bryanph thank you for taking the time to write that up! I really appreciate it. Those are great questions, I'll try to answer each one...

So what would be possible with this architecture that isn't possible currently? I'm not sure I fully understand the issue.

Good question. This would unlock a few different things:

I think that is it. But it also is about cleaning up the model distinctions that currently make the codebase confusing, and that limit it from achieving things in the future.

If this controller is not a React component then how will we handle making properties like History pluggable?

Written up above, but it comes down to adding "commands" and "queries" for this.

Will there be a method that takes plugins as an input and return a collection of events to apply to the React editor and a schema? PluginArray => { Schema, EventMap } And the result will be applied to the React Editor component?

Essentially, yes. Except I'm thinking about trying to make it more transparent, such that all of the commands are exposed as first-class methods on the Change object. This keeps the API native feeling and terse, instead of having to result to a secondary data structure, or

But I think it would be good to expose editor.commands with a map of them so that people can check to ensure that their plugins are registering properly. I haven't really fleshed this out yet.

Since the schema defines what values the Document can contain, doesn't that make it part of the Model?

This one is tricky. It defines what shape the document (and later ideally the selection too) can take. But it isn't necessarily part of the model so much as a constraint that is applied to the model. It leads to confusion if a value can have a value.schema when inserted into the editor, while at the same time the editor has an editor.schema. And then there's a question of which one is the "true" schema, or if they are somehow combined, etc.

Instead, if the editor is allowed to control the schema, those questions disappear. This matches more closely with the ideas being developed like isVoid or isAtomic not being inherent to the model, but inherent to the editor. One editor could treat images as voids in the traditional sense, whilst another could decide that it edits their textual content by leaving them non-void with the image as a background-image. (Or something 😄 else crazy!)

I thought perhaps the environment-specific code like adding event listeners could be specified in the plugin as belonging to a certain environment, like React or React native. ... However, thinking about it further, you'd probably want to keep these plugins separate, so have a slate-react-plugin-list and a slate-react-native-plugin-list, with some duplicate code between them for extending the schema, normalization, etc. ... But if we want to support React Native in the future we'd probably want to start naming plugins to reflect that they only work in React and not in React Native

These are good points. I think we're not quite close enough to seeing what React Native, or what Vue support looks like to make a decision. But it could very well be that we end up separating the fields out by environment if we need to. Same for the plugin naming. Until we get closer though I think it's safe to keep them as-is.

How does all that sound?

zhujinxuan commented 6 years ago

Shall we have editor.onProps (for props update) and editor.onChange (for flush change) to bind the editor model to the ` react component?

ianstormtaylor commented 6 years ago

@zhujinxuan yup! Currently I've got the Editor controller accepting an onChange function for calling back, and exposing its own editor.setProperties method for updating the changed properties.

ianstormtaylor commented 6 years ago

Some more thoughts, as a result of sketching some of this out with code...

We have the concepts of isAtomic and isVoid. Right now these are solved by the schema, with an isVoid: true or isAtomic: true entry. This is fine, but I also wonder if these aren't just two of the core "queries" that could be answered. The rest of the schema deals with validation, and this is a small outlier.

It could also be solved by allowing plugins to answer queries, and you'd achieve the same functionality. Not necessarily that that's better, because it might be more of a hassle to write, but it's just an observation of an existing code path that could be eliminated.

Further, you can actually model the existing normalizeNode plugin definitions in the same way. It could also be a "query" that plugins can choose to answer. And it would work exactly as it currently does, without having to be special-cased.