Extract ActionText::Editor base class and ActionText::TrixEditor adapter

[seanpdoyle]

Related to rails/actiontext#41

The introduction of the ActionText::TrixEditor class enables the
deprecation of a variety of class- and instance-level methods across the
ActionText namespace. Action Text benefits from deprecating methods in
order to reduce its public API.

There are two categories of deprecations in this changeset:
module/class deprecations and method deprecations.

The module and class deprecations target modules that are internal
and "private" in intent, but public in Ruby and not marked as :nodoc:.
They're either particular to Trix, or their responsibilities have been
given to ActionText::Editor.

The method deprecations also target methods that are internal and
"private" in intent, but public in Ruby and not marked as :nodoc:.
In general, determining whether or not to deprecate a method hinged on
whether its name or arguments mentioned "trix", or if it lacked
method-level documentation or comments of any kind.

When possible, the previous code paths have been deprecated and
re-implemented in terms of an ActionText::TrixEditor instance.

Additions

The main aim of this changeset is to provide a single, extensible
entrypoint for third-party editors (that are not Trix) to integrate with
Action Text.

The initial responsibilities of the ActionText::TrixEditor have been
determined by consolidating a variety of (class- and instance-) methods
across a variety of classes to reduce the number of touchpoints for
applications and engines that provide third-party editor integrations.
The vast majority of its method definitions are direct copy-and-pastes
from their original sources. Any method that previously mentioned trix
in its name or argument list has replaced and generalized that
occurrence with editor.

Changes

These changes aim to be considered implementation details, and intend
to remain "private" from an API perspective. Future commits can be made
to expand upon and document the responsibilities of the adapter's
interface.

Checklist

Before submitting the PR make sure the following are checked:

• This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
• Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
• Tests are added or updated if you fix a bug or add a feature.
• CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

[seanpdoyle]

This change aims to both introduce the ActionText::Editor class as the central hub for all rich text processing and rendering and deprecate the old methods.

If that's too ambitious, risky, or the code review diff is too noisy, I'm happy to open a separate PR to deprecate the methods, then rebase this branch off that PR.

[swanson]

For example, this proposal draws inspiration from the ActiveStorage::Service class and its related Registry and Configurator infrastructural classes to store per-record information about the editor used to create its constituent data.

What would be a use-case for having this kind of configuration for ActionText? I can understand the case for ActiveStorage having multiple different "services" (mirroring, different regions, public vs private files, etc) but struggling to think of a case where a single Rails app would use multiple different text editor libraries. In my own codebase we have different "instances" of trix (for things like removing some formatting, changing styles, etc) but those are configured at the view-layer or with options passed to the underlying library.

[seanpdoyle]

struggling to think of a case where a single Rails app would use multiple different text editor libraries.

I think one potential use case is migration from one editor to another. For example, this diff's migration would backfill existing records to set editor_name = "trix", then any new records could set editor_name = "prosemirror".

For some period of time, the application could serve assets for both editors and use the editor_name column to determine which context to render the content in. It could render edit forms with Trix, then do some transformation when Trix submits the Trix-specific HTML content into the shape the new editor expects.

For some editors, there might not be any significant difference. Since the WYSIWYG landscape is vast, supporting an open-ended migration felt safest.

In my own codebase we have different "instances" of trix (for things like removing some formatting, changing styles, etc) but those are configured at the view-layer or with options passed to the underlying library.

This is another case I was imagining. Some consumer-facing pages might require real-time collaboration features, while other pages (back of house administrative pages, for example) might have different needs. While I agree with your point that using a consistent tool across the entire application would be the simplest approach, the spirit of this proposal is to provide flexibility when that isn't possible (for a variety of reasons, both in and out of teams' control).

[swanson]

My gut reaction was that I was expecting this to be configured/designed more like ActiveJob (different adapters, a per-has_rich_text editor option, config.action_text options) and not like ActiveStorage. I have not explored that path in any depth, strictly speaking as a consumer of ActionText. Either way, very excited about this! It's a big undertaking!

[seanpdoyle]

a per-has_rich_text editor option

This is an angle I hadn't considered. I think supporting editor: :trix and editor: :prosemirror could have value. Also supporting a callable could help with migrating from one to another. Something like:

has_rich_text :body # editor defaults to `ActionText::RichText.editor`
has_rich_text :body, editor: :trix
has_rich_text :body, editor: ActionText::RichText.editors[:trix]
has_rich_text :body, editor: ->(rich_text) { rich_text.method_to_determine_editor_during_migration } 

[ideasasylum]

We migrated from Trix to TipTap over the course of 2023. Our app pre-dated ActionText but this type of PR could help other apps in a similar situation to ours:

1. We needed to re-create our own version of the RichText model and ActiveStorage integration. This was necessary because ActionText was tied to Trix so we couldn't just adopt ActionText but our implementation doesn't really do anything different.
2. It was a year-long process so we were concurrently running Trix and TipTap in different parts of the app, and even within the same part of the app as the migration process ran.

[MatheusRich]

@dhh shedding some light on this PR since you've mentioned working on a possible Markdown new editor for Rails and you were open to this idea at first.

[jorgemanrubia]

This looks amazing @seanpdoyle 👏

[jorgemanrubia]

I would leave this out of this PR to keep the size under control, but some thoughts about multi-editor support.

To support using multiple editors in the same app, we should be able to create thread-safe contexts where you can configure the editor. So that here, in the helpers you use in your app, we could support an editor: option that let you opt out the default. With that option, you could create contexts where ActionText::RichText.editor is resolved differently. So, some API like:

ActionText.with_editor :trix do
   # ...
end

ActionText.with_editor :lexxy do
   # ...
end

That way, within the helpers, we could set the right "editor context" to interpret the editor param. To do this, we would need to make the editor variable a thread one.

We need this because through the code we rely on ActionText::RichText.editor resolving to the configured editor, which is handy.

[seanpdoyle]

Could this involve an implementation that leverages Object#with like:

module ActionText
  singleton_class.delegate :editor, :editor=, to: "ActionText::RichText"

  def self.with_editor(editor_name)
    with editor: ActionText::RichText.editors.fetch(editor_name) do
      yield
    end
  end
end

I think the singleton_class.delegate bit is necessary since ActionText::RichText is an Active Record model, and Active Record overrides Object#with with ActiveRecord::QueryMethods.with for common-table expression support.

[seanpdoyle]

@jorgemanrubia I'd declared the RichText.editors and RichText.editor class attributes because I thought they read as self-descriptive (since they read aloud as "rich text editors" and "rich text editor").

Other than hooking into an ActiveSupport.on_load(:action_text_rich_text) { … } block in the engine, there isn't much benefit to declaring the attributes on RichText, rather than ActionText itself.

Is that decision worth reconsidering before merging?

[jorgemanrubia]

Hey @seanpdoyle some proposed changes here. It reverts the last WIP commit and introduce the proposed changes in another commit:

1. Pass the rich text content back and forth at the adapter level. I know I pushed for using HTML strings, but that certainly introduces an innecessary parsing cycle on every editor load operation. The failing test felt more like a symptom than a cause here. Sorry that my original instinct caused trouble here, as I think you originally advocated for passing content fragments back and forth 🙏.
2. Don't use the editor adapter when rendering rich text content. That felt wrong, conceptually. Action text is stored canonically and it should be renderable as it is.
3. Clarify the editor API flow by introducing canonical and editable, to enforce the symmetric nature of both:

module ActionText
  class Editor
    def as_canonical(editable_content)
      editable_content
    end

    def as_editable(canonical_content)
      canonical_content
    end
  end
end

The tests passed for me in my branch. There are two tests I deleted when reverting the WIP, but I guess we want to keep those.

Could you get your branch up to date with these changes? I will be happy to finally merge this. Thanks so much 🙏🙏.

[seanpdoyle]

@jorgemanrubia I've pushed 8582d55 to restore the test coverage and to propose one final adjustment to the Editor interface.

[seanpdoyle]

This current changeset combines two units of work:

1. one unit that deprecates any method that mentions trix in its name, and replaces it with a more generic editor version
2. one unit that creates new classes in support of pluggable editors

@jorgemanrubia now that the Editor interface feels more settled, I wonder if it would be more productive to split out the deprecation unit (1.) into its own PR to be reviewed and merged on its own. I find that the renames and deprecations add a lot of noise to the diff that makes it more substantial than it needs to be.

We can keep this PR intact to serve as the pluggable editor unit of work (2.).

What do you think?

[jorgemanrubia]

@seanpdoyle this looks great 👍👍. I think in a single unit is fine.

Thanks for the fantastic work here.