Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve plugins design #3222

Open
focux opened this issue Dec 4, 2019 · 10 comments
Open

Improve plugins design #3222

focux opened this issue Dec 4, 2019 · 10 comments

Comments

@focux
Copy link

focux commented Dec 4, 2019

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

Feature

What's the current behavior?

Right now it's so hard to enclose logic together inside a plugin due to the lack of method like e.g. onKeyDown

Slate: 0.50

What's the expected behavior?

I think the philosophy behind plugins or what makes it a great pattern in software is that it helps us to enclose all the logic that belongs to a single feature together, in one place and actually, that's where his name comes from: plug-in.

Today I was writing a mention plugin and I notice how my code was dispersed through many files, making it hard to understand and far away from what a plugin is.

I think that we should add methods like onKeyDown to the editor interface, to help us improve and enclose more our plugins.

Also, our goal with plugins should be that we only need a plugin to implement a complete feature, this way the community is going to be able to enrich more the Slate ecosystem through plugins to do many things.

That's my humble opinion.

@cameracker
Copy link
Collaborator

This has come up in chat a few times. Here are some links for posterity:
https://slate-js.slack.com/archives/CC58ZGGU9/p1575306369039100

https://slate-js.slack.com/archives/C1RH7AXSS/p1575393968376400

The short of it is that this is a pretty specific design decision that factors in the onBeforeInput event handler, and also the way React Flare will work going forward.

@cameracker
Copy link
Collaborator

Also, our goal with plugins should be that we only need a plugin to implement a complete feature, this way the community is going to be able to enrich more the Slate ecosystem through plugins to do many things.

One more thing I'll add is that this is easier said than done. The more we abstract into a plugin and hide from a potential user, the more likely a user is going to have to deal with behavior conflicts. Consider a plugin that provides schema rules for the document structure. How do we manage a case where multiple plugins provide conflicting rules? What happens when a plugin provides some features that you don't want, but doesn't provide a mechanism to turn these off? You have to fork right? That defeats the purpose of these plugins a lot of the time.

One major advantage to "some assembly required" is that the end user of the plugin gets more hands on control with how the plugin is integrated into their editor ecosystem. Even if the plugin is huge and has a lot of behaviors that are potentially undesirable, now it's easier to turn them off.

I think ultimately this new plugin user experience is not that much different to how a typical library is consumed. A user will have to export some functions, read some examples on how they're hooked up. I understand that the "plug and play" experience is not there anymore in quite the same way, but I think largely rich text editing toolkits require careful attention to detail so this might be better in the long run.

I also disagree with the implied assertion that plugins that require some additional hook up will be harmful to the ecosystem. We can still have a rich pool of plugins that provide a great deal of high value behaviors. I'm skeptical that this is going to be a material disincentive for plugin maintainers to produce plugins. The folks who would be producing plugins will still do so regardless of whether the consumer needs to do additional hook up.

@focux
Copy link
Author

focux commented Dec 4, 2019

How do we manage a case where multiple plugins provide conflicting rules?

It's our job to figure that out, honestly, I haven't been using Slate for so long.

What happens when a plugin provides some features that you don't want, but doesn't provide a mechanism to turn these off?
That's why plugins have config options, you can take Webpack's plugins as an example.

I think there are good products with good plugins systems that we can learn from: Webpack, Figma, NextJS(WIP).

@ianstormtaylor
Copy link
Owner

Hey @focux thanks for opening this. I understand the frustration. I think there's definitely a missing piece right now for making it easier to tie interaction into inputs to Slate's editor. I'm just not sure that putting them in plugins is the way to do it.

@focux: I think there are good products with good plugins systems that we can learn from: Webpack, Figma, NextJS(WIP).

Definitely share here what you think they are doing well that we can take. I'd be interested to hear more about Next's and Figma's. (Although I don't really want to copy anything Webpack has done because I think it's my least favorite piece of software 😄)

I'm not sure that plugins by definition have to be able to override every single behavior. We could call them something else if that's people's impression. It seems like a way to guarantee that they become amorphous and hard to reason about.

Why No Events?

Like @CameronAckermanSEL mentioned, there were reasons that the event-related logic was taken out of plugins. Some of them have to do with how modern browsers are moving away from pure keyboard inputs, some have to do with how React's future seems to involve less direct DOM event usage, and some have to do with ensuring that plugins are flexible.

Modern browsers, especially mobile ones, have a lot of rich text input types that no longer map 1-to-1 to keyboard events. And when composition due to IME is concerned things get even murkier. In earlier versions of Slate plugins would often be overriding the wrong triggers to provide functionality. Using commands instead makes it clear what they should be overriding.

For example, earlier plugins would often listen for keydown events of -Space for trigger "auto-list-item" logic. But this becomes leaky on platforms that don't trigger events for spacebar. Or, listening for CmdB for bold, which doesn't catch all cases where a beforeinput with input type of formatBold occurs (eg. right-click or the B button in the iOS toolbar). There are lots of leaky things that happen when listening to a single DOM event to capture intent because of cross-browser compatibility.

I think I'd like to keep Slate's editor mixins separate from the complexity of DOM events.

We've kind of forgotten how hard it is to manage incompatible events for the majority of app-related events because they've been standardized fairly well by browsers these days. And before they were, React's own Synthetic Events system paved over them for us.

If the Synthetic Events were extendable I'd probably have done that.

But it seems like React is attempting to solve some of this with their "Flare" (for example the usePress hook) project. It's still in unstable territory, but it seems to be moving to using Hooks instead of classic DOM event handlers to listen for events. One of the benefits of this is abstracting over the DOM event layer to potentially have a one-to-many relationship between intent and events. Hopefully it will be extendable, it seems like it will.

(React's Synthetic Events already work this way actually—they are often one-to-many without us realizing it.)

Using Hooks?

I could imagine a future where we have a hook like useBold:

useBold({
  onBold: () => editor.exec({ type: 'toggle_bold_mark' })
})

Which help you ensure you're handling all the cases by listening for either:

  • CmdB
  • beforeinput when inputType === 'formatBold'
  • ...anything else?

Potentially this could be something that slate-react maintains in it's own codebase, to make it easy for people to stay consistent, since I agree that right now it's a huge pain to get all of the cases for DOM events right.

Implementing Mentions

I think mentions is one of the hardest things to implement in the current architecture. And I think it's because it has a lot of complications...

The standard way to do it involves keeping "state" in the actual content when a user types @ma for instance on their way to typing a whole name. And then it also needs to toggle open/closed some sort of suggestions list. And not only that, but then map the standard keyboard events inside the <Editable /> to controlling the active state of the suggestions. That's a lot of overlapping concerns.

It's much harder than other floating toolbar-type use cases where once the toolbar is opened it can listen to its own events until it's closed. But it also has never been easy to implement I feel like, even in prior versions of Slate it seemed like the feature people struggled with the most.

"Feature" Plugins?

Previously Slate recommended grouping "features" together into a single plugin. But I'm not sure this was a good idea in retrospect, for a couple reasons.

One thing is that there are often "helpers" that a given feature needs. If it's lists then you might want to have a indentList, outdentList, wrapList, unwrapList, etc. And these things are schema-specific, because they depend on the naming schemes that the user has chosen.

Previously in the case of things like GitBook's slate-edit-list plugin these would be return after calling a function. But this is really awkward. It makes it very hard for people to keep track of them all in one place. It makes much more sense for the helpers to be exported directly from the package and then pass in the configuration when calling them instead.

Also problematic was the idea of "disabling" plugins. If a plugin contains all the logic for lists, this includes the schema rules and commands. When you "disable" it, you don't actually want the schema for list items to be removed. You still want to ensure that any existing list items are correctly formed. But you might instead want to tweak your top-level schema to remove any list items it finds. Similarly, you don't want a format_list command to suddenly not exist as a concept anymore, you just want to turn it into a no-op.

Further, if people want to use plugin-specific behaviors on the server-side, it's problematic that the plugins contains tons of DOM-related and React-related logic.


Anyways, I'd love to hear other people's takes on this. What other options there might be for us to make it easier for people to implement these more complex behaviors. Or even how plugins might be improved to make some of this stuff easier—hopefully with coupling concerns too much.

I don't claim that we've gotten it right yet, just that there were a handful of reasons for making the change. In general I think they make things easier to reason about. But it might take adjusting to figure out how to approach them in the new architecture.

For anything that was possible before but now isn't I'd be curious to hear. Hopefully we can figure out a way to solve it.

@whitphx
Copy link

whitphx commented Dec 17, 2019

Hello,

I appreciate the current plugin design which excludes DOM-related features
and would like to agree with it from a different but related perspective to the statement

Further, if people want to use plugin-specific behaviors on the server-side, it's problematic that the plugins contains tons of DOM-related and React-related logic.

I have written many unit tests of my editor logic and they are fully independent from either DOM or even virtual DOM thanks to the separation.
It makes tests easier because

  • there is no need to emulate DOM/virtual DOM to render components and to handle DOM events.
  • all assertions are applied to pure JS objects (e.g. editor object and its children), not rendered DOMs.

And since the logics are encapsulated into the DOM-independent code and exposed only via commands and helpers, React-related code that only calls the commands can be simple enough to skip writing tests on it.

This design decision makes slate much more easier to write tests on user-land logics than other rich text editor libraries and this is one of the reasons I love slate.


Though, I also agree with the problem mentioned here, that not all related code cannot be enclosed into a single package.
For that, I created separated my-slate-editor-plugin and my-slate-editor-react-helpers packages like the core slate and slate-react ones.
I think it's a tradeoff we have to accept that there exist 2 packages: DOM-related and DOM-unrelated.

@zbeyens
Copy link
Contributor

zbeyens commented Dec 30, 2019

@focux
I published a package slate-plugins-next with plugins, helpers and a component EditablePlugins on top of Editable that accepts plugins like before v0.50. There is 20+ plugins extracted from the official examples. You can find docs and demos on Storybook.
https://slate-plugins-next.netlify.com/?path=/docs/*
https://github.com/zbeyens/slate-plugins-next

@CameronAckermanSEL

Consider a plugin that provides schema rules for the document structure. How do we manage a case where multiple plugins provide conflicting rules? What happens when a plugin provides some features that you don't want, but doesn't provide a mechanism to turn these off? You have to fork right? That defeats the purpose of these plugins a lot of the time.

That's a very true statement about plugins. I tackled this problem by using the atomic design.

I see "feature" plugins as molecules: a group of atoms mixed together to form a feature, where an atom is the smallest piece of logic (i.e. renderElement or renderLeaf of a specific feature). I also see "editor plugins" like withMention, withPasteHtml as atoms.

The idea is that we should not restrict the editor design to only atoms (>= 0.50) or only molecules (=< 0.47). Let Editable be an organism that can compose both molecules and atoms.

You can even improve the flexibility of your molecules and atoms by using optional parameters (=< 0.47). For example, most of the time we want to provide our own React component to a Link plugin implementing renderElement.

To conclude, I see plugins at least as nice-to-have for quick prototyping. It's not an easy task to build generic cross-platform feature plugins.

@ianstormtaylor
Copy link
Owner

Here’s a more recent PR that shows the direction React is moving with using hooks for events: facebook/react#17651

@michaelcuneo
Copy link

I've had to rewrite everything... and I love it. This new way is insanely better.

@Obiwarn
Copy link

Obiwarn commented May 15, 2020

OK, so just to be clear: Right now there is no "official" way to refactor out sth. like the mentions feature out of the editor? (I tried and it is crazy complicated).

Only way right now: I could use the slate-plugins-next plugin (but I fear the "lock in").

@cdaz5
Copy link

cdaz5 commented Oct 3, 2020

I'm a very recent user of slate (so I dont claim this is a great implementation and obviously nowhere near "official" @Obiwarn) but figured i'd mention it in case others come here looking for a possible solution. Throughout our app we plan to have an editor in multiple places accomplishing different tasks (in some places it'll make use of onFocus others it wont, some need a mentions like search some dont, etc.).

The thing I didnt want to do was copy paste the editor for each use case then slightly customize it and have a lot of dup code that would require exponential maintenance if they both need the same "feature" (like if two instances needed to use the mention search) and said feature then suddenly needs to be adjusted slightly at the request of product team now we have to change the code in many places potentially.

So i came up with a solution like this (its pretty much a copy paste of the mentions example related code tweaked for our use case)

and then create the component outside of wherever we want to use it const Editor = withEntitySearch(MyEditor)
and render it out const SomeScreen = () => (<Editor />)

this leaves <MyEditor /> super bare bones (the only "mentions" related code in it is this check:
{withEntitySearch && ( <EntityOptions entityOptions={entityOptions} target={target} ref={optionsMenuRef} searchState={searchState} index={index} setTarget={setTarget} /> )}
and we only have to maintain the mention search in one place. Its by no means perfect, bc we wrap a couple times withEntitySearch(WithSomeOtherThing(MyEditor))

this does mean if more than one wrapper wants to use the same event we need to check for it and call it
const onKeyDown = (e) => { if (props.onKeyDown) props.onKeyDown(e) // then this wrappers logic }

and if the wrapper itself needs to create the editor we also have to do this in the generic editor file:
// slateEditor is the editor passed from the search wrapper
const editor = useMemo(() => slateEditor ?? withHistory(withReact(createEditor())), [slateEditor]);

i think this could also get unwieldy if we have a lot of wrappers utilizing the same events, but i dont think we will and it at least keeps the wrappers code isolated so if we need to make changes to it we only have to change it in one place.

I should note, we only did this bc we also didnt want to use a library like slate-plugins-next (its looks great and no offense to it) but since slate itself is in beta we didnt want to lock ourselves into another 3rd party lib. If we didnt care about this i'd prob use that library. Lastly, i'll mention i've only started using slate about a week ago, so all of this might be super naive approach until i fully understand the api 🤷‍♂️ .

Hope this helps someone and also if anyone comes up with a better mousetrap please share!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants