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

[lexical] Chore: Added missing isInline function to TextNode #7226

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mrdivyansh
Copy link
Contributor

Description

LexicalNode expects the extender Node to implement isInline. However, it is missing from TextNode implementation.

isInline(): boolean {
invariant(
false,
'LexicalNode: Node %s does not implement .isInline().',
this.constructor.name,
);
}

Test plan

Before

  • Invoke isInline function on a TextNode instance.
  • Invariant will throw an error -- LexicalNode: Node TextNode does not implement .isInline().

After

  • Invoke isInline function on a TextNode instance.
  • The function should return true.

Copy link

vercel bot commented Feb 21, 2025

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 Feb 21, 2025 5:39pm
lexical-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 21, 2025 5:39pm

@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 Feb 21, 2025
@mrdivyansh
Copy link
Contributor Author

Not sure if this PR requires any unit tests.

@mrdivyansh mrdivyansh changed the title [Lexical] Chore: Added missing isInline function to TextNode [lexical] Chore: Added missing isInline function to TextNode Feb 21, 2025
@etrepum
Copy link
Collaborator

etrepum commented Feb 22, 2025

This might make sense, but there should already be guards to prevent this from happening on nodes other than ElementNode and DecoratorNode. Did you run into a situation where this exception occurs? If so, that’s where the unit test should be. I think otherwise it might make more sense to move this to the ElementNode and DecoratorNode interfaces specifically because those are the only two types of nodes where it may return something meaningful.

/**
* @returns true if the text node is inline, false otherwise.
*/
isInline(): boolean {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this does make sense to implement on TextNode, it would make sense to also narrow the implementation to return only true, because it's not permitted for any subclass to return false

Suggested change
isInline(): boolean {
isInline(): true {

@etrepum
Copy link
Collaborator

etrepum commented Feb 24, 2025

Also if we do decide to implement this, there's a fourth node that extends LexicalNode that should implement isInline: LineBreakNode. That would cover all of the expected cases, since the value for LexicalNode isn't exported there's no reasonable way to add another class to the hierarchy.

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.

3 participants