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

Fix - withReact type signature #5091

Merged
merged 3 commits into from
Aug 22, 2022

Conversation

e1himself
Copy link
Contributor

@e1himself e1himself commented Aug 19, 2022

Description
withReact() plugin type signature is inaccurate.

In reality, it accepts any editor instance implementing BaseEditor and extends it with additional ReactEditor API.

But the type definitions require to pass an instance that is already a ReactEditor.

The typical workaround is either to cast the instance with as ReactEditor or define the Editor custom type in the project. However, both are band-aid solutions to the real problem: invalid type spec of the withReact() function.

Issue
Fixes: #4144

Example

Before the fix:

Screenshot from 2022-08-19 17-38-17

After the fix:

Screenshot from 2022-08-19 17-38-45

Context

n/a

Checks

  • The new code matches the existing patterns and styles.
  • The tests pass with yarn test.
  • The linter passes with yarn lint. (Fix errors with yarn fix.)
  • The relevant examples still work. (Run examples with yarn start.)
  • You've added a changeset if changing functionality. (Add one with yarn changeset add.)

@changeset-bot
Copy link

changeset-bot bot commented Aug 19, 2022

🦋 Changeset detected

Latest commit: 002e738

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

This PR includes changesets to release 1 package
Name Type
slate-react Patch

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

@dylans
Copy link
Collaborator

dylans commented Aug 20, 2022

So this looks fine to me. I do believe there was a deliberate reason why they deliberately kept ReactEditor in BaseEditor but I'm not remembering what that was. Leaving this open for a couple of days in case anyone remembers why.

@e1himself
Copy link
Contributor Author

e1himself commented Aug 22, 2022

I do believe there was a deliberate reason why they deliberately kept ReactEditor in BaseEditor but I'm not remembering what that was.

I am not sure if there was a reason.

If you look at the code before the patch:

export const withReact = <T extends Editor>(editor: T) => {
const e = editor as T & ReactEditor

it's completely not obvious (or even unexpected) that it will result in this Typescript definition:

export declare const withReact: <T extends ReactEditor>(editor: T) => T & ReactEditor;

Note how <T extends Editor> became <T extends ReactEditor>.

@e1himself
Copy link
Contributor Author

Leaving this open for a couple of days in case anyone remembers why.

👍

Thanks.

@dylans dylans merged commit e18879e into ianstormtaylor:main Aug 22, 2022
@github-actions github-actions bot mentioned this pull request Aug 22, 2022
@e1himself e1himself deleted the fix/with-react-type-signature branch August 24, 2022 09:26
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.

Error in signature of withReact
2 participants