Skip to content

fix: insertblocks when no type is provided #1436

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

Merged
merged 4 commits into from
Feb 17, 2025
Merged

Conversation

YousefED
Copy link
Collaborator

This addresses a regression after multi-column where insertBlocks (and blockToNode) would throw an error if no block type was provided in the partialblock. Also added unit test

closes #1408

Copy link

vercel bot commented Feb 17, 2025

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

Name Status Preview Updated (UTC)
blocknote 🛑 Canceled (Inspect) Feb 17, 2025 1:31pm
blocknote-website 🛑 Canceled (Inspect) Feb 17, 2025 1:31pm

@@ -281,7 +281,10 @@ export function blockToNode(

const nodeTypeCorrespondingToBlock = schema.nodes[block.type];

if (nodeTypeCorrespondingToBlock.isInGroup("blockContent")) {
if (
!nodeTypeCorrespondingToBlock || // can happen if block.type is not (this should create the default node)
Copy link
Contributor

@nperez0111 nperez0111 Feb 17, 2025

Choose a reason for hiding this comment

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

This could be a little clearer. Maybe something more like:

const nodeTypesCorrespondingToBlock = schema.nodes[block.type ?? 'blockContent']

Or, whatever makes sense as the default value

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

see update, the issue is that schema.nodes[undefined] is valid javascript but ofc. returns undefined. TS would say the returntype would be NodeType which is invalid.

To fix this we'd need to make the index signature of nodes return NodeType | undefined with a PR to pm-model. I think that's a bit overkill here so I now typed nodeTypeCorrespondingToBlock explicitly. wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea, agreed that isn't necessary. But, I was more referring to that since block.type could be undefined, it should not be used to index the schema.nodes object, and should instead use whatever fallback would be there (so you don't even need the !nodeTypeCorrespondingToBlock || [XX] part.

Not a big deal though, the explicit type helps make it clearer a bit

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good point, I think it's better now

@YousefED YousefED requested a review from nperez0111 February 17, 2025 12:50
@YousefED YousefED merged commit 8429d62 into main Feb 17, 2025
4 checks passed
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.

InsertBlocks does nothing
2 participants