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 "TypeError: Cannot use 'in' operator to search for '__language' in null" in LexicalSelection #5912

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

Conversation

gigantedocil
Copy link

@gigantedocil gigantedocil commented Apr 17, 2024

Prevents the error "TypeError: Cannot use 'in' operator to search for '__language' in null" from being thrown if firstBlock is null by putting the $isElementNode(firstBlock) first in the if statement.

Copy link

vercel bot commented Apr 17, 2024

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 Apr 17, 2024 7:49pm
lexical-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 17, 2024 7:49pm

@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 Apr 17, 2024
@@ -1209,7 +1209,7 @@ export class RangeSelection implements BaseSelection {
const last = nodes[nodes.length - 1]!;

// CASE 1: insert inside a code block
if ('__language' in firstBlock && $isElementNode(firstBlock)) {
if ($isElementNode(firstBlock) && '__language' in firstBlock) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If it is indeed null then the postfix non-null assertion operator is also wrong and should probably be removed so typescript can help sort out some of the other edge cases (if there are any)

-const firstBlock = $getAncestor(firstPoint.getNode(), INTERNAL_$isBlock)!;
+const firstBlock = $getAncestor(firstPoint.getNode(), INTERNAL_$isBlock);

Copy link
Author

Choose a reason for hiding this comment

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

Makes sense, @etrepum . I removed it, but now I'm getting this error on line 1264:

image

Is it enought to check if firstBlock exists in the if statement on line 1264? Or does this need some other type of logic?

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not too familiar with what this code is trying to do and what its expectations are, ignoring the null here would make the type check pass but it might be hiding another bug. @Shubhankerism @zurfyx and @GermanJablo made changes to this function recently and might have a better idea of how to properly fix all of it.

Copy link
Contributor

Choose a reason for hiding this comment

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

It would help to know under what circumstance firstBlock is null. In principle it should not happen, since it is a RangeSelection that is not in root (something that is checked at the beginning).

Copy link
Author

Choose a reason for hiding this comment

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

@GermanJablo, thanks. This is what I was trying to achieve:

image

Copy link
Contributor

Choose a reason for hiding this comment

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

CustomTextNode and CustomParagraph extend the TextNode and ParagraphNode classes, right?
Could you make a repro in codesandbox or stackblitz?

Copy link
Author

Choose a reason for hiding this comment

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

CustomTextNode and CustomParagraph extend the TextNode and ParagraphNode classes, right?

Yes, exactly.

Could you make a repro in codesandbox or stackblitz?

I'll try to do it during the weekend, whenever I have some free time.

@s0rta
Copy link

s0rta commented May 16, 2024

are your custom nodes inline or block?

@gigantedocil
Copy link
Author

@s0rta, both. I have a CustomParagraphNode that extends ParagraphNode and a CustomTextNode that extends TextNode.

@s0rta
Copy link

s0rta commented May 23, 2024

@gigantedocil would you want to throw your nodes into this sandbox? I tried to create a minimum replication and couldn't get it to fail, so there's a different bug somewhere up the call stack for me. But my initial fix was setting the custom node's isInline to true

@gigantedocil
Copy link
Author

Thanks, @s0rta. I'll try to do this whenever I have some free time.

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.

None yet

5 participants