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

Refactor editor methods and fix JSDoc #5307

Merged
merged 29 commits into from
Apr 20, 2023

Conversation

zbeyens
Copy link
Contributor

@zbeyens zbeyens commented Feb 22, 2023

Description

This pull request introduces a new pattern without any breaking changes. All Editor and Transforms methods now call editor methods. For example, Transforms.setNodes is now calling editor.setNodes, and editor.setNodes is now calling setNodes, an exported function implementing the default editor behavior. You can override editor.setNodes on your end so that you can either use Editor.setNodes or editor.setNodes, and both will use your overridden behavior, and Slate will too.

The editor object now has many more methods, and some changes have been made to improve code style consistency. None of the implementation of the methods has changed, except for the addition of a new argument to editor.insertText.

This PR also includes:

  • Moving JSDoc's to the interface type to allow IDEs access to the interface methods.
  • Removing method types where type inference can be used to reduce redundancy.
  • Improving code style consistency.

Example

Please refer to the example code snippet below for the usage of the new pattern:

import { createEditor } from 'slate'
import { withReact } from 'slate-react'

const customSetNodes = editor => {
  // Your custom behavior here
}

const editor = withReact(createEditor())

editor.setNodes = customSetNodes

// Now, if you (or Slate) uses Transforms.setNodes or editor.setNodes, it will use your overridden behavior.

Context

This pull request allows for more flexibility in implementing custom behavior for Editor and Transforms methods without breaking any existing functionality. It achieves this by introducing a new pattern where Editor and Transforms methods call editor methods, and editor methods call an exported function implementing the default editor behavior.

This pattern allows for overriding editor methods with custom behavior, and using either Editor or editor methods, which will both use the overridden behavior, and Slate will too.

ReactEditor is not yet following that pattern. I'd wait for this pattern approval first.

With the introduction of many new methods to editor, it may be beneficial to consider implementing a plugin-based architecture to prevent overloading the editor object. This would involve relocating all queries to editor.queries and all transforms to editor.transforms. However, this is a significant breaking change that is not included in this PR.

@changeset-bot
Copy link

changeset-bot bot commented Feb 22, 2023

🦋 Changeset detected

Latest commit: e6e8724

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
slate-react Minor
slate Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

# Conflicts:
#	packages/slate/src/create-editor.ts
#	packages/slate/src/interfaces/editor.ts
# Conflicts:
#	packages/slate-react/src/components/string.tsx
#	packages/slate/src/interfaces/editor.ts
# Conflicts:
#	packages/slate-react/src/components/editable.tsx
# Conflicts:
#	packages/slate/src/interfaces/editor.ts
#	packages/slate/src/transforms/selection.ts
#	packages/slate/src/transforms/text.ts
@zbeyens zbeyens marked this pull request as ready for review April 15, 2023 11:57
@zbeyens zbeyens changed the title Feat/overrides Refactor editor methods and improve IDE integration Apr 16, 2023
@zbeyens zbeyens changed the title Refactor editor methods and improve IDE integration Refactor editor methods and improve JSDoc Apr 16, 2023
@zbeyens zbeyens changed the title Refactor editor methods and improve JSDoc Refactor editor methods and fix JSDoc Apr 16, 2023
@12joan
Copy link
Contributor

12joan commented Apr 16, 2023

Does this have implications for the withX pattern like we use in Plate? I imagine something like this could be used now instead of the old pattern:

import { someFunction } from 'slate[-react]';

export const withX = (editor) => ({
  ...editor,
  someFunction: (x, y, z) => {
    console.log('someFunction', x, y, z);
    return someFunction(x, y, z);
  },
});

@zbeyens
Copy link
Contributor Author

zbeyens commented Apr 16, 2023

@12joan This composition pattern is now possible, indeed! However, in that example, the last plugin would override the other plugins overrides (on the same method), so we'd need to slightly change Plate core logic to support the piping pattern.

# Conflicts:
#	packages/slate/src/create-editor.ts
@TheSpyder
Copy link
Collaborator

This change bloated the minified core by 19% and destroyed one of the things I really liked about slate; a focus on static method implementations meant any features I didn't use were tree-shaken away.

I'm disappointed to see Slate moved in this direction while I was away. I don't think it meshes with the goals Ian set out for it. Surely you could've done this all-in-one just with the slate-react editor instance? Maybe having a spaghetti bundle with all features linked as methods on the editor object is fine if you're using slate-react, but that's not something I plan to do.

@dylans
Copy link
Collaborator

dylans commented May 20, 2024

This change bloated the minified core by 19% and destroyed one of the things I really liked about slate; a focus on static method implementations meant any features I didn't use were tree-shaken away.

I'm disappointed to see Slate moved in this direction while I was away. I don't think it meshes with the goals Ian set out for it. Surely you could've done this all-in-one just with the slate-react editor instance? Maybe having a spaghetti bundle with all features linked as methods on the editor object is fine if you're using slate-react, but that's not something I plan to do.

Yikes, ok. Plan wasn’t to bloat/break tree shaking, but to try to make things more consistent. Will look into this asap.

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

Successfully merging this pull request may close these issues.

None yet

4 participants