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

feat: Expose getActiveEditor and getActiveEditorState #5409

Merged
merged 4 commits into from
Jan 25, 2024

Conversation

yf-yang
Copy link
Contributor

@yf-yang yf-yang commented Dec 21, 2023

Partially implements feature request #5333

Submit a pull request and waiting for the team's opinion.

By the way, do we have any bot that builds this commit online so that I can directly install this commit via npm? Need this one to access parent editor during importJSON for my own purpose.

Copy link

vercel bot commented Dec 21, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
lexical ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 18, 2024 4:21am
lexical-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 18, 2024 4:21am

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Dec 21, 2023
@acywatson
Copy link
Contributor

If we do expose them, I'd like to add typedoc doc blocks to them so they show up in the API docs and people have a sense of how they work (e.g., what is the concept of "active" editor)

@yf-yang
Copy link
Contributor Author

yf-yang commented Jan 3, 2024

@acywatson I've added two $-prefixed functions. Do you have any comments on the naming/doc?

Comment on lines 1631 to 1633
export function $getEditorState(): EditorState {
return getActiveEditorState();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pending editor state was always meant to be kept private (by design), that's why we provide a rich set of utilities to modify its content and should be treated as an implementation detail. It's also one of the mutable parts of Lexical. Hence, it is dangerous to expose externally.

Can you provide more details on why you think it's important to expose this please?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I agree with you. Let me try to summarize my thoughts:
$getEditorState is almost equivalent to $getRoot and $getSelection, so it is not necessary.
However, by exposing $getEditor, then one can always do $getEditor().getEditorState(), which is equivalent (isn't it? not so sure), so I just added it here without too much thoughts.
I am OK to remove this API, but your comment makes me consider if exposing $getEditor is a good idea. The only reason to expose $getEditor is to access the editor from within a node because you can always do so when calling editor.update or editorState.read by accessing the parent scope. What's your opinion?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$getEditor().getEditorState() always returns the rendered EditorState (as opposed to the pending). $getRoot allows you to read or manipulate the rendered or pending editor state without any knowledge on whether the EditorState or nodes have been cloned. That is the reason why we're inclined to think that pending editor state should always be kept private!

Copy link
Member

@zurfyx zurfyx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for iterating on this, LGTM!

@zurfyx zurfyx merged commit e511326 into facebook:main Jan 25, 2024
34 of 45 checks passed
@yf-yang yf-yang deleted the feat/expose-utils branch March 11, 2024 08:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants