-
-
Notifications
You must be signed in to change notification settings - Fork 529
refactor: blocknote API uses lowest possible level prosemirror primitives #1609
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
Conversation
This reverts commit ffc4a5b.
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@blocknote/ariakit
@blocknote/code-block
@blocknote/core
@blocknote/mantine
@blocknote/server-util
@blocknote/react
@blocknote/shadcn
@blocknote/xl-docx-exporter
@blocknote/xl-multi-column
@blocknote/xl-odt-exporter
@blocknote/xl-pdf-exporter
commit: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool! Do you think you have a preference after these explorations? Specifically looking at which codebase / architecture would be the most maintainable / strongest to work with down the line?
packages/core/src/api/blockManipulation/commands/updateBlock/updateBlock.ts
Outdated
Show resolved
Hide resolved
packages/core/src/api/blockManipulation/commands/moveBlocks/moveBlocks.ts
Outdated
Show resolved
Hide resolved
@@ -1073,18 +1076,28 @@ export class BlockNoteEditor< | |||
targetBlock: BlockIdentifier, | |||
placement: "start" | "end" = "start" | |||
) { | |||
setTextCursorPosition(this, targetBlock, placement); | |||
return this.transact((tr) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are some of the methods like this one wrapped in transact
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
transact
is convenient in that you don't have to create a transaction & then dispatch it. It also is clear that it would work within a blocknote transaction.
If everything were to use this pattern, then you wouldn't even need to have editor.transaction
hold the activeTransaction
state, it would just be passed around through the transact
calls. So, it ends up mapping more closely to what actually happens
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah that makes sense! I was looking at it from the angle of "only call transact
if you're doing multiple BlockNote API calls in a method.
But you're right, we could also use it for convenience + make the methods consistent. I suppose in that case we need to do it for all usages of dispatch
, and perhaps even make dispatch
private?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call on standardizing it, will refactor to that pattern. I think it will come out nice
I prefer the explicitness of what we have here, but I think the tradeoff that it makes are methods that need to take a ton of arguments, which is a code smell imo. Take a look at this method: BlockNote/packages/core/src/api/blockManipulation/commands/updateBlock/updateBlock.ts Lines 67 to 78 in 34ebafa
It's annoying that it needs to take 2 schemas (blocknote & prosemirror) and the blockCache. I think that this could be aleviated though. The prosemirror schema has a cached object which can store the blockCache & blocknote schema inside of it. This would make most methods only ever need a |
ok! let's go for this approach then 👍
We could also just create a type |
I think I like this better. It turns out that from a transaction's document, you can get a schema. So storing it into the schema makes it really convenient to reach for values you may need. Just as a POC, I cleaned up the insertBlocks method with this. I think the API is a lot cleaner, and nodeToBlock & blockToNode become much simpler too: 2980c6c |
ok! Let's go and see how this works in practice :) |
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
This refactors all of the blocknote APIs to use low-level prosemirror APIs rather than passing around editor instances and dispatching transactions themselves.
Now, block manipulation functions will only update the current transaction it was provided rather than trying to dispatch it. This allows these commands to be chained without having to resort to any smarts for the transaction handling.
supercedes: #1147 #1604 #1584
Resolves: #829