RFC: Behavior #64

marijnh · 2018-12-13T17:41:18Z

This branch contains an implementation of a mechanism that replaces (state) plugins. It works roughly like this:

A behavior is an object with <Spec, Value> type parameters
The Spec parameter is the thing that you pass when loading the behavior, usually a configuration object
The Value parameter is the thing that consumers of the behavior, if any, can get access to (though the behavior object's get method)
There's two kinds of behaviors—those that manually merge their specs into a single value, and those that just accumulate an array of them (Value=Spec[])
Instead of plugins, you now pass behaviors parameterized with specs when creating a state
Behaviors can specify sub-behaviors, and loading a behavior pulls those in
For behaviors that combine their specs (such as history), a single set of sub-behaviors is used for the entire behavior. For those that accumulate them separately (such as keymap), every spec results in a separate set of sub-behaviors
When initializing a state, for each behavior, an ordered set of specs is found. For behaviors that combine their specs, the combining function gets the specs in this order. For set behaviors, the set ordering is determined this way
Each use of a behavior can specify a priority from a set of 4 pre-programmed priorities
The ordering happens first by priority, then by order, where the order is determined by the order in which the behaviors were given when creating the state, with sub-behaviors inheriting the position of their parent behavior
Unless explicitly specified, a sub-behavior inherits the priority of its parent

The main things that this tries to accomplish is:

Introduce a generic kind of 'services' that extensions can provide (via a state) and other code can consume
Allow behaviors to compose other behaviors without causing problems when behaviors are pulled in twice via separate paths
Provide more control (and access to) the ordering of plugins/behaviors, so that you can for example override a key binding without fine-tuning the order of your plugins (and so that you can override one behavior without having all the behaviors you provide be high-precedence)

The approach is a little baroque (though I already put a lot of effort into keeping it minimal), but it seems to meet these goals well. I have, for example, introduced an indentation behavior that the legacy modes now provide, along with generic commands that do indentation, which use this behavior. I haven't tried to wire up tokens with bracket matching yet, but that should be able to work the same way.

I'd love to hear your comments, @adrianheine

state/src/behavior.ts

adrianheine · 2018-12-14T18:27:40Z

state/src/behavior.ts

+    if (defaults) for (let key in defaults)
+      if (result[key] === undefined) result[key] = (defaults as any)[key]
+    return result
+  }


I think combineConfigs should rather be a stand-alone function. Behavior already has two different kinds of static properties: The define(Set) factory functions and the stateField / multipleSelections / viewPlugin / indentation well-known Behaviors, and combineConfig doesn't fit in there and confuses me.

These kind of utility functions are always hard to find a good place for. It's somewhat dubious whether this belongs in state at all, but I needed it several times for the existing plugins, so I guess it's nice to provide it. I'm okay with moving it to a stand-alone function. Our doc generation tool should be flexible enough for us to be able to put item docs in sections (as with builddoc's src/README.md system), so then we can still make sure that function appears near Behavior in the docs).

adrianheine · 2018-12-14T18:43:24Z

state/src/behavior.ts

+Behavior.viewPlugin = Behavior.defineSet()
+Behavior.indentation = Behavior.defineSet()
+
+export class BehaviorSpec {


This name confused me: I would expect a SomethingSpec to be an abstract thing with which you can create Somethings, but Behavior and BehaviorSpec are the other way round?

Agreed. Naming remains hard. Any suggestions? BehaviorInstance? BehaviorUse?

I thought we could just switch the names, but (the current) Behavior will be used much more than (the current) BehaviorSpec, so I think calling that BehaviorUse is better.

Right, using Behavior for the main point of interaction was a pragmatic thing. I'm okay with BehaviorUse

adrianheine · 2018-12-14T19:09:27Z

state/src/behavior.ts

+  get(state: EditorState): A<Spec> {
+    return state.config.behavior.get(this) || none
+  }
+}


Maybe consider removing this subclass and move the check to the callers (currently commands and editorview). Or instead generalize a default value concept for non-set behaviors, too?

I also consider having this class a bit ugly, but the alternatives seem more problematic. For non-set behaviors, being present or not are distinct conditions, and thus we'd require some kind of special null value there, not a default.

Maybe make get error when called on a non-set behavior that isn't present, and provide a different accessor to check for availability?

marijnh · 2018-12-15T09:05:32Z

Thank you for your input! Does the overhaul as a whole look like an improvement to you?

Another wart that I'm not happy with is the term 'set' (as in defineSet and SetBehavior) since it's actually more of an array, and it should also express the separateness of the values (which get their own dependent behaviors)... but I haven't been able to find a word that covers that. Ideas?

marijnh · 2018-12-16T12:19:07Z

I've implemented the solution to get rid of BehaviorSet as a class, and moved combineConfig to the top level. Good to merge?

adrianheine · 2018-12-16T13:11:07Z

That looks nice. To be honest, I was just playing around with a different direction: I noticed that behavior's and getBehaviors value argument is any, but shouldn't be. I tried moving takeType into Behavior as a property wrapping combine and behavior and in a second step made Behavior an abstract base class with two subclasses.

As for merging, I'm still playing around with this to better understand it. So far I've mostly looked at the implementation, not usage. I'm still quite confused by the terminology and the fact that behaviors are used for implementing both plugins and small composable aspects of plugins. That's elegant, but difficult to wrap my head around. I'm quite confident that we will merge this and move in this direction, but I would like to play with it a bit more and maybe rename or move around things. If it makes your work easier we can merge it and continue on master.

adrianheine · 2018-12-16T13:16:58Z

I pushed my branch because I think I really like the way the two types of Behaviors are encapsulated: https://github.com/codemirror/codemirror.next/blob/behavior2/state/src/behavior.ts.

marijnh · 2018-12-16T13:33:11Z

As for adding a type parameter to BehaviorUse / BehaviorSpec (85e3d68), I had it that way initially, but it doesn't actually help type client code (you don't do anything with such an object except feed it to a new state), and neither does it help much in the implementation code (you're filling in any pretty much everywhere), while still complicating a user-visible type, I don't think it's a helpful change.
Similarly, behavior.behavior is internal, so the kludge in the way it is typed doesn't really help client code, and adding more classes to work around it seems hardly worth it.

marijnh · 2018-12-16T13:34:34Z

(Think of this as unsafe in Rust—doing weird but correct things internally while providing a well-typed, clean interface is usually fine.)

adrianheine · 2018-12-16T14:45:29Z

The BehaviorSpec type parameter would have helped me reading the code. I would have preferred using a Behavior type as type parameter but I don't know if it's possible to refer to a generic type parameter's type parameters.
I don't think of my approach as a kludge. Instead, I think the way you currently basically sub-type and branch in takeType based on whether combine is given is very hard to read and surprising. Currently, I'm not even able to concisely describe Behaviors interface and behavior, because most of it resides in takeType.

Maybe we can look at smaller steps first: What do you think about moving the behavior call into takeType and only calling it once for non-set behaviors?

I'll try to focus on the user-facing side of things now to provide more useful feedback.

adrianheine · 2018-12-16T18:45:59Z

I think a lot of my initial confusion can be solved by renaming things:

default is a default config, default_config is quite long, but maybe it would be ok?
behavior returns sub-behaviors (except for behaviors where the behavior itself doesn't do anything)
combine instantiates the Value
Behaviors are both instances of Behavior and the Values those instances resolve to (now I get were BehaviorSpec came from)

marijnh · 2018-12-16T19:28:00Z

I guess I should have written up proper docs (and comments) before creating this pr—was so glad to finally have something that worked that I just had to rush it onto github.

behavior returns sub-behaviors

Yes, they were called dependencies before, but that doesn't really capture it (these are behaviors injected by the parent behavior, which is something quite different from what dependencies in module systems tend to be). subBehaviors seemed awkward, so I abbreviated to behavior, as in, "the behavior implied by this parent behavior". Definitely needs docs, but given docs, do you think this is too cryptic?

Behavior used to be called BehaviorType, but again, that felt a bit cumbersome, since it's the main class you'll be interacting with. I feel there might be a better way to frame this, but I haven't figured it out yet.

At one point I had a design where plugins and behaviors were different, so that plugins were the top-level things you included and each could provide any number of behaviors, but that seemed both too restrictive (you'd also need dependencies on plugins to do things like plugins including keymaps) and introducing more concepts than necessary (in the end there's no fundamental difference between them, and it's often convenient to abbreviate a simple plugin as just a behavior).

I'm okay with defaultConfig (or defaultSpec). It's just a named parameter anyway, in the public interface, and that one conveniently doesn't conflict with the default keyword.

adrianheine · 2018-12-16T19:41:05Z

I think it's alright for me to figure things out this way. It's really the flexibility that gets me: In history, behaviors implement a factory with config merging and one dependency; In keymap and legacyModes, it's just mapping one config at a time to a set of behaviors; gutter, matchBrackets and specialChars merge config and then evaluate to a single viewPlugin; multipleSelections too but also adds a marker behavior. That makes it really difficult to find names for concepts or even understand the concepts in the first place.

adrianheine · 2018-12-16T19:51:06Z

Are there use-cases where you would want to get a behavior not only based on the type, but also spec?

adrianheine · 2018-12-16T19:55:41Z

Does it make sense to even store the value of behaviors that only emit configuration? Currently you only get the values for indentation, history, viewPlugin, stateField and multipleSelections. Those are pretty obvious and I cannot think of a reason you would need the others that are currently available.

adrianheine · 2018-12-16T20:24:07Z

(Just throwing thoughts and questions around) Quick overview over the currently defined behaviors:

                    own behavior  subbehavior combine
history             x             field       custom
keymap              -             multiple    set
legacyModes         -             multiple    set
gutter              -             view        simple
matchBrackets       -             view        simple
specialChars        -             view        simple
multipleSelections  -             view, mS    custom

stateField          x             -           set
multipleSelections  x             -           custom
indentation         x             -           set
viewPlugin          x             -           set

marijnh · 2018-12-17T08:58:34Z

Are there use-cases where you would want to get a behavior not only based on the type, but also spec?

I don't think so.

Does it make sense to even store the value of behaviors that only emit configuration?

No. My thinking is that it's mostly harmless to allow this, and it helps keep things simple, but this may be a pointer in the direction of a design change. Especially since, if the Value type of a behavior isn't public, but the behavior is, you'll be forced to either export the Value type or cast it to any when exporting the behavior, which is awkward.

adrianheine · 2018-12-17T09:05:12Z

knownSub could move to a separate data structure in BehaviorStore::resolve, since it's only used there.

marijnh · 2018-12-17T09:05:54Z

knownSub could move to a separate data structure in BehaviorStore::resolve, since it's only used there.

Sure, but why add another mapping when a private field suffices?

adrianheine · 2018-12-17T09:08:28Z

To make Behavior easier to understand for me :)

adrianheine · 2018-12-17T09:10:46Z

I think the most helpful change for me would be to make it explicit when a ›behavior‹ provides, uhm, behavior itself as compared to only specifying subbehaviors.

marijnh · 2018-12-17T09:11:29Z

(I think I generally prefer comments to additional code, when it comes to making the program easy to understand — at least those won't have to be loaded and evaluated by every user.)

adrianheine · 2018-12-17T09:13:40Z

I find that picking things apart (at least in my mind) that don't necessarily belong together helps me finding better approaches.

marijnh · 2018-12-17T09:15:49Z

Thinking about this one concretely, those known sub-behaviors have to persist, so I don't know where they'd be stored if local to resolve. In a module-global variable?

adrianheine · 2018-12-17T09:16:57Z

Persist between what? Multiple resolve calls?

marijnh · 2018-12-17T09:17:21Z

Yes.

adrianheine · 2018-12-17T09:23:40Z

So (naming is just a sketch) if we would have something like:

keymap = Behavior.usages<Keymap>({
  behaviors: configs -> [viewPlugin.use(/*...*/)]
})
gutter = Behavior.singletonUsage<GutterConfig>({
  combineConfig: configs -> config,
  behaviors: config -> [viewPlugin.use(/*...*/)]
})
stateField = Behavior.define<StateField<any>>()
history = Behavior.defineSingleton<HistoryConfig, HistoryBehavior> {
  combine: configs -> config,
  create: config -> HistoryBehavior,
  subBehaviors: HistoryBehavior -> [stateField.use(/*...*/)]
}

… that wouldn't be as elegant, but – provided we find good names for the factory functions – easier to understand.

adrianheine · 2018-12-17T09:25:35Z

Not that I think this discussion is particularly relevant, but storing the known sub-behaviors across multiple resolve calls is just an optimization, right?

marijnh · 2018-12-17T09:26:00Z

(Yes, it is.)

So that it can be properly typed. (This was an issue we had with the old plugin system that was accidentally fixed by behaviors!)

And have them explicitly mark their target type in a type parameter, in preparation for using these in view plugins.

To reduce the number of concepts client code has to import, and to make things that can be plain functions actual plain functions.

marijnh · 2019-01-14T16:11:33Z

I've rebased this on top of the current master branch, and made some more adjustments.

The idea of these was to introduce less concepts and types (see #67). Basically, I've made it so that consumers can import a single class, and have everything they need (except for those that store a behavior store, those need to also import BehaviorStore). This class is called Extension and plays the role of what used to be Extender in the previous iteration.

The biggest change is that everything that can be a raw function is now a raw function (behaviors and extensions), without a custom type slapped on top of it. Plain, non-unique extensions can even be written as plain functions (i.e. defineExtension just vanishes). Behaviors still need to be defined (but defineBehavior returns a plain function), and unique extensions still need to be wrapped with Extension.unique.

Instead of putting the Target type as a type parameter in every extension-related type (which was a way to prevent you from applying view extensions to a state and vice versa, which I expect will be a rather common mistake to make), I'm now using a run-time check. Different exendable systems are expected to create their own subclass of Extension, and when a behavior created with subclass A is seen in the resolve method of subclass B, it raises an error. Dynamic errors are, on the whole, less nice than static typechecking errors, but this introduces a lot less cruft in the types (and also works in a non-TypeScript setup), so I think it's an improvement.

adrianheine · 2019-01-15T12:04:22Z

Although I usually like my type system to do the work it's supposed to do, I agree that we need some dynamic type checks where we interface with (possibly non-TS) user code.

adrianheine · 2019-01-15T12:25:53Z

gutter/src/index.ts

-import {EditorView} from "../../view/src"
+import {StateExtension} from "../../state/src"
+import {combineConfig} from "../../extension/src/extension"
+import {EditorView, viewPlugin} from "../../view/src"


Suggested change

import {EditorView, viewPlugin} from "../../view/src"

import {EditorView, PluginView, viewPlugin} from "../../view/src"

adrianheine · 2019-01-15T12:26:05Z

gutter/src/index.ts

+export const gutter = StateExtension.unique<GutterConfig>(configs => {
+  let config = combineConfig(configs)
+  return viewPlugin(view => new GutterView(view, config))
+}, {})

 class GutterView {


Suggested change

class GutterView {

class GutterView implements PluginView {

That makes sense, but PluginView is on the way (already is, locally) to be replaced by several view behaviors, so it doesn't matter.

Ok, how does gutter look like in that version? Currently, it's confusing that viewPlugin's argument is allowed to return some arbitrary class.

It looks like this:

export const gutter = ViewExtension.unique<GutterConfig>(configs => { let config = combineConfig(configs) return ViewExtension.domEffect(view => new GutterView(view, config)) }, {})

So the 'arbitrary class' thing is still there, but that's how interfaces work.

And domEffect doesn't expect an interface GutterView can implement?

I don't understand the question... it does exactly that, which is why that code typechecks, no?

Ahh, I see, you want implements declarations. Yeah, you could add implements DOMEffect on the class.

That's good :) In the current (old) version, it's obvious that we are following the framework's (hu) interface, but in the new, more terse version, I don't immediately recognize that.

adrianheine · 2019-01-15T12:28:55Z

commands/src/commands.ts

+}
+
+function getIndentation(state: EditorState, pos: number): number {
+  for (let f of state.behavior.get(StateExtension.indentation)) {


I think indentation would be nicer as a standalone import.

Why? Does this go for all of the static values on StateExtension? (Would it make sense to continue this discussion in #67 ?)

I think it goes for all of them. Maybe because they are not extensions, but behaviors? I think indentation should be indentationGetter or getIndentation or indentationAt and they could all have a Behavior suffix for clarity.

At this point, behaviors can be considered to be a type of extension.

These are not getters—if you call them, you create an extension that holds the value you pass.

No, they are not getters, but the behavior provided by indentation is »getting the indentation at a specific position«. state.behavior.get(StateExtension.indentation) returns an array of functions that return the indentation at a specific position.

adrianheine · 2019-01-15T13:24:53Z

history/src/history.ts

 }

-export function redoSelection({state, dispatch}: {state: EditorState, dispatch: (tr: Transaction) => void}): boolean {
-  return historyCmd(PopTarget.Undone, ItemFilter.Any, state, dispatch)
+const historyBehavior = StateExtension.defineBehavior<HistoryBehavior>()


This is confusing. Maybe we can have a simple wrapper like this:

Extension.adhocBehavior = function<Value>(value: Value): Extension { let behavior = Extension.defineBehavior<Value>() return behavior(value) }

EDIT: Wait, you still need to be able to retrieve the behavior, right.

Why is it confusing? And indeed, that wrapper would define an anonymous behavior that you can never access.

I think it's rather HistoryBehavior that confuses me and I just wrote a comment over there. For the other, exported behavior types, this method is not confusing.

adrianheine · 2019-01-15T13:36:32Z

history/src/history.ts

-export function undo({state, dispatch}: {state: EditorState, dispatch: (tr: Transaction) => void}): boolean {
-  return historyCmd(PopTarget.Done, ItemFilter.OnlyChanges, state, dispatch)
-}
+class HistoryBehavior {


This class is mainly a handle so that the commands and depth getters don't have to close over the configuration (and hence, field). I think the code would be easier to understand if this were either

split in two behaviors (HistoryCommand, HistoryDepth) or

reduced to only storing the two fields with the content of the methods moving down into cmd and depth.

Pint 2. is a good idea. 1. seems like it wouldn't help much while introducing another separate element. Renaming the class to HistoryContext or so might cover its purpose relatively well.

See fc5b290

adrianheine · 2019-01-15T13:40:13Z

This definitely needs some getting used to from my side and there's probably a lot more changes that will happen, but I think we are at a point where merging would simplify our work.

marijnh · 2019-01-15T13:43:42Z

I agree on merging soon. I'm close to finishing the view behavior part (I hope), which has motivated several adjustments to this stuff already. I think when that works, I'll merge and we can address further issue with separate discussion/prs.

marijnh · 2019-01-15T14:33:34Z

I've merged this + view behavior into master. Some notes about the new code:

Given that extensions could be applied at both the state and the view level, it became hard to figure out what kind of extension to export from view-extending packages. If you export a view extension, it can't be put into a state without wrapping it, but if you export a state extension, you can't directly use it with a view.

Extending an editor using some imported stuff is the kind of code that every user of the library will end up interacting with, so it should be smooth. My solution is to allow any kind of extension (Extension type) to be passed when creating a state—the state extensions will be integrated in the state, and the non-state extensions stored in state.behavior.foreign, from which the view picks them up to try and apply them locally. If after resolving the view extensions, any extensions remain, an error is raised since that probably means you passed a state extension to the view.
The mechanism for keeping state in the view, and deriving display-relevant 'slots' from it (which currently means only decorations, but to which I'll soon add wrapper attributes and eventually also stuff like gutter widgets) is indeed awkward and I'm still thinking about how that should look. The constraints are that it should be possible to keep one state and derive multiple 'slots' from it (since often the place that creates decorations is also the sensible place to create gutter widgets), but it should also be easy to just compute decorations from states and updates without adding extra machinery (as in StateExtension.decorations).
The way extensions are passed to the view will definitely change.

adrianheine reviewed Dec 14, 2018

View reviewed changes

state/src/behavior.ts Outdated Show resolved Hide resolved

adrianheine reviewed Dec 14, 2018

View reviewed changes

marijnh and others added 15 commits January 14, 2019 17:03

Wire up basic indentation commands using Behavior.indentation

0caf231

Remove .some accessor from SetBehavior

43e4f96

Stricter typing for BehaviorSpec

7ce7d40

Rename BehaviorSpec to BehaviorUse

cff18c2

Make Behavior.get a partial function, drop BehaviorSet class

817468c

Make combineConfig a top-level function

cba9d18

Move viewPlugin to the view module

b775ecd

So that it can be properly typed. (This was an issue we had with the old plugin system that was accidentally fixed by behaviors!)

Implement separation between extensions and behaviors

563aedd

Port the rest of state/ to adjust extension interface

9195df6

Make all the code use Extension

4ca5f28

Comment extension.ts a bit better

943201b

Make behaviors and extensions functions

737e4ee

And have them explicitly mark their target type in a type parameter, in preparation for using these in view plugins.

Move behavior subsystem out of state/, so that view/ can also use it

c901cf6

Port extensions and behaviors to yet another approach

5594398

To reduce the number of concepts client code has to import, and to make things that can be plain functions actual plain functions.

Move behavior tests to the extension package

52dd467

marijnh force-pushed the behavior branch from 489dedd to 52dd467 Compare January 14, 2019 16:04

adrianheine reviewed Jan 15, 2019

View reviewed changes

marijnh added 2 commits January 15, 2019 15:19

Use behavior-style extensions for the view

37786d6

Clean up use of local behavior in history

6fb612c

adrianheine closed this Jan 15, 2019

adrianheine deleted the behavior branch January 15, 2019 19:02

	import {EditorView, viewPlugin} from "../../view/src"
	import {EditorView, PluginView, viewPlugin} from "../../view/src"

RFC: Behavior #64

RFC: Behavior #64

Conversation

marijnh commented Dec 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marijnh commented Dec 15, 2018

marijnh commented Dec 16, 2018

adrianheine commented Dec 16, 2018

adrianheine commented Dec 16, 2018

marijnh commented Dec 16, 2018

marijnh commented Dec 16, 2018

adrianheine commented Dec 16, 2018

adrianheine commented Dec 16, 2018 • edited Loading

marijnh commented Dec 16, 2018 • edited Loading

adrianheine commented Dec 16, 2018

adrianheine commented Dec 16, 2018

adrianheine commented Dec 16, 2018

adrianheine commented Dec 16, 2018

marijnh commented Dec 17, 2018

adrianheine commented Dec 17, 2018

marijnh commented Dec 17, 2018

adrianheine commented Dec 17, 2018 • edited Loading

adrianheine commented Dec 17, 2018

marijnh commented Dec 17, 2018

adrianheine commented Dec 17, 2018

marijnh commented Dec 17, 2018

adrianheine commented Dec 17, 2018

marijnh commented Dec 17, 2018

adrianheine commented Dec 17, 2018

adrianheine commented Dec 17, 2018

marijnh commented Dec 17, 2018

marijnh commented Jan 14, 2019

adrianheine commented Jan 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marijnh Jan 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrianheine Jan 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrianheine Jan 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrianheine commented Jan 15, 2019

marijnh commented Jan 15, 2019

marijnh commented Jan 15, 2019

adrianheine commented Dec 16, 2018 •

edited

Loading

marijnh commented Dec 16, 2018 •

edited

Loading

adrianheine commented Dec 17, 2018 •

edited

Loading

marijnh Jan 15, 2019 •

edited

Loading

adrianheine Jan 15, 2019 •

edited

Loading

adrianheine Jan 15, 2019 •

edited

Loading