-
-
Notifications
You must be signed in to change notification settings - Fork 526
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(core): add blocknote transactions #1584
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
9db70e3
to
4ce1f42
Compare
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.
Copilot reviewed 32 out of 32 changed files in this pull request and generated no comments.
4ce1f42
to
4a3d364
Compare
@blocknote/ariakit
@blocknote/code-block
@blocknote/core
@blocknote/mantine
@blocknote/react
@blocknote/server-util
@blocknote/shadcn
@blocknote/xl-docx-exporter
@blocknote/xl-multi-column
@blocknote/xl-odt-exporter
@blocknote/xl-pdf-exporter
commit: |
/** | ||
* The state of the editor when a transaction is captured, this can be continuously updated during the {@link transact} call | ||
*/ | ||
private transactionState: EditorState | null = null; |
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.
are we sure we should store an EditorState
, or a transaction
? any trade-offs?
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.
Always a trade-off 😄
The trade off here is that we now have the possibility of "desync"-ing between the tiptapEditor state and our transaction state.
But, this is exactly the same sort of approach that Tiptap command chains make (which is why it is always important in a tiptap command to read from the command state, not the editor state).
I think this actually works out pretty well for us, since we want people to be using the blocknote API instead of lower-level APIs like reading from the view state. Ideally, a consumer should not ever need to know about low-level details like that.
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.
The only way to implement a transaction mechanism is for the editor state to be temporarily de-coupled from the view state, so that a longer transaction can be "chained"/"built up". An alternative to this sort of API is Tiptap's chained fluent API where the transaction isn't applied until a specific call is made.
Personally, I like the chained API approach, but it forces your code to be rewritten to it, rather than your code leveraging it. It's like the difference between React & jQuery. React is not portable, but jQuery is
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.
The only way to implement a transaction mechanism is for the editor state to be temporarily de-coupled from the
view state
An alternative is to not "de-couple" the state at all, but explicitly expose a transaction like I did in #1147, right? afaik you don't need any magic / chaining at all, as long as your methods can operate on prosemirror transactions. I think this is the most prosemirror-native approach?
Trying to establish the pros / cons:
This PR
- you can "magically" use
prosemirrorState
, and in most cases "everything should just work". - pro: you can always read prosemirrorState after a dispatch
- con:
prosemirrorState
is a bit magic / might not reflect the actual state of the editor (view). This could cause errors down the line
#1147
- requires methods to be more aware of a transactions; i.e.: you should continue with an existing transaction if that's available on the editor, and read from
tr.doc
. This might be more cumbersome, but normally would all be abstracted away for consumers - con: a
dispatch
call won't result in an updatedprosemirrorState
if you're in a transaction - pro: because it's more explicit, maybe less "risky" / error-prone?
The approaches also differ in plugin handling but if I see it correctly we could use any plugin approach with both of the solutions.
I'm not sure I have a clear preference. I like the ease of being able to access prosemirrorState
in one approach, but it also feels more on the safe side (and easier to reason about the code paths) to do as little "magic patching" as possible especially in the core internals.
I might also be missing some pros/cons, so curious to your thoughts
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.
An alternative is to not "de-couple" the state at all, but explicitly expose a transaction like I did in #1147, right?
Sorry, yes that is right.
I tried that approach when implementing this PR. I was able to implement everything at one point to use the current transaction as the source of truth for the state.
While this did work, it didn't feel "right" to start a transaction when just trying to read the current state. I would be more comfortable with this approach if, we did not expose the editor.prosemirrorState
at all, and chose to only expose the editor.transaction
. This would prevent confusion between doing the "right thing" (editor.transaction) and the "wrong thing"(editor.prosemirrorState), by only exposing the right thing.
Additionallly, I think that this would work if we had full control of all the callsites, but what is slightly problematic about this approach is external code.
For example, take this random command from prosemirror-table
which takes an editor state to execute the command, if called by us in a transaction, we have to generate an editor state for it to be called appropriately (with whatever is actually the most recent state). I think this is something that we should be solving for our callers, our job is to shield complexity from developers by making it a non-issue.
public transact<T>(callback: () => T): T { | ||
if (this.transactionState) { | ||
// Already in a transaction, so we can just callback immediately | ||
return callback(); |
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.
I have the feeling we're not compatible with multiple nested transact
calls. If I see it correctly, while "entering" should work, on "exit", we'd immediately dispatch when we exit the nested transaction.
If this is correct, I think we either need to fix it, or just throw an error here whene entering a nested transaction
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.
Pretty sure that the logic is good here. The goal here is to only actually dispatch the active transaction until anything within the callback has been run. Which, I think is achieved here, even with nested calls.
Take this example for reference:
❯ node
Welcome to Node.js v22.14.0.
Type ".help" for more information.
> let inTr = false
undefined
> function transact(cb) { if (inTr) { return cb() }
... inTr = true;
... const result = cb();
... console.log('dispatch');
... inTr = false;
... }
undefined
> (() => {
... console.log("entering tr");
... transact(() => {
... console.log("in tr", inTr);
... console.log("entering nested");
... transact(() => {
... console.log("in nested", inTr);
... });
... console.log("finished nested", inTr);
... });
... console.log("done with tr", inTr);
... })();
entering tr
in tr true
entering nested
in nested true
finished nested true
dispatch
done with tr false
undefined
Nested transactions should always be hitting the if (this.transactionState) {}
branch, since they are within an outer transaction which would set this to a truthy value.
They'd have no "exit", since their callback is immediately executed, so they would never hit the automatic dispatch of the active transaction.
So, I'm pretty sure the logic is fine
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 ofc, you're right. might be worth a test case but definitely misinterpreted this
public dispatch(tr: Transaction) { | ||
if (this.transactionState) { | ||
const { state, transactions } = | ||
this.transactionState.applyTransaction(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.
this would apply plugins, right?
- do we want this?
- Should we check whether all plugins are compatible with this? i.e.: the state that they get will be different from the "actual editor state" (as the actual editor - or at least, the view - still has an old state)
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.
This would apply the filterTransaction
and appendTransaction
events.
Both of these have function signatures provide the current editorState as an argument filterTransaction and appendTransaction. So operating on the editor state they were given is what they should be doing anyway (since they are run during state transitions), it would be incorrect for them to be pulling anything from editor.prosemirrorState
, editor._tiptapEditor.state
or editor._tiptapEditor.view.state
.
I've checked our plugins and this should not be an issue, I did not have to change any of the plugins for this.
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.
We also need the plugins to apply because after an editor.dispatch
the plugins would normally be assumed to have already run, and we don't want to break this assumption just because the parent called the code within a transaction.
/** | ||
* Get the underlying prosemirror state | ||
*/ | ||
public get prosemirrorState() { | ||
if (this.transactionState) { | ||
return this.transactionState; |
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.
this can now be not in sync with the view. can this cause downstream issues you think?
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.
The states can now not be in sync. This is the main trade-off. The issues that can occur are minimal though:
- reading from the wrong state variable could you give the wrong state (e.g. reaching into
_tiptapEditor.state
should be discouraged because of this), this could lead to incorrect content being read, or incorrect positions. Both, you'll notice pretty quickly - There is risk that a transaction is never "completed" and the state never gets applied to the editor again. I tried to minimize this with a
try { } finally { }
block which should mean it should always exit out of this state.
So, the only way to guarantee that things work within a transaction is to read from either the current transaction
state (e.g. tr.doc
, tr.selection
), or in the worst case to read from editor.prosemirrorState
which will calculate the appropriate transaction state given the current "active transaction". I purposefully didn't enforce everything to read from just the transaction
state as that can sometimes lead to awkward code (like starting a transaction when you are just trying to read a node's position in the doc).
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.
Thanks. Another thing that could go wrong is calling dispatch
and then trying to find the node in the view (dom), right? For example, getting an element by id. Previously this would have worked, so I suppose this is also something to check / be mindful of?
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.
Another thing that could go wrong is calling dispatch and then trying to find the node in the view (dom), right?
Yea, I would bucket that with the first statement:
reading from the wrong state variable could you give the wrong state (e.g. reaching into _tiptapEditor.state should be discouraged because of this), this could lead to incorrect content being read, or incorrect positions. Both, you'll notice pretty quickly
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.
Seems like @YousefED already covered the design decisions so just added a few questions about a few bits of code.
If I understand correctly, the main difference/point of contention between the BlockNote transactions and the underlying ProseMirror ones is:
- In PM you create a new set of transactions from
state.tr
, in which you should only refer totr.state
, before applying them withstate.applyTransaction
. - In this PR, you call
editor.transact
to create/add to a set of transactions, after which you can refer toeditor.prosemirrorState
like usual before applying them witheditor.dispatch
.
So the advantage of this is that as a developer, you don't have to worry about transaction state vs editor state, and just have the editor state exposed to you. But the downside is you need to make sure that after calling editor.transact
, you need to call editor.dispatch
ASAP or the editor state gets desynced.
Am I getting that right?
tr.steps.forEach((step) => { | ||
accTr.step(step); | ||
}); | ||
if (tr.selectionSet) { |
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.
How come selections need their own case?
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.
Selections aren't stored as a Step
, they are stored in a separate field. So, to preserve selections between, we have to copy it over separately.
You'll also notice that I serialize it to JSON, because technically the document in the dispatch'd transaction is a different reference than the accumulated activeTransaction
Sorta, you are right about the advantages, reading
|
This implements BlockNote transactions, which batch multiple dispatched transactions into 1 transaction
supercedes: #1147
Resolves: #829