Feature: add ability to call custom transaction functions #16

2020-01-09T01:53:35+01:00

jschen5 commented

2020-01-09 01:53:35 +01:00

(Migrated from github.com)

We'd like to be able to abstract away complex operations on transactions and still be able to chain them as part of a sequence of operations along with other simpler operations. Here's a simple example use case.

function insertTextOrSetSelection(tr, text, anchor, head, doInsert) {
    return doInsert
        ? tr.insertText(text) 
        : tr.setSelection(TextSelection.create(tr.doc, anchor, head))
}

// what we want to be able to do
state.tr
    .chain(insertTextOrSetSelection, 'first', 1, 2, getDoInsert())
    .insertText('second')

// current workaround
const tr = state.tr
insertTextOrSetSelection(tr, 'first', 1, 2, getDoInsert())
tr.insertText('second')

You could imagine the branching logic within the custom function being a lot more complex, and maybe you'd even want to be able to chain multiple of them together.

Since this is my first time contributing to prosemirror, I wasn't sure if we want features to have an associated ticket in the github issue tracker, or only bugs. If so, I'll create one and link it to this.

We'd like to be able to abstract away complex operations on transactions and still be able to chain them as part of a sequence of operations along with other simpler operations. Here's a simple example use case. ``` function insertTextOrSetSelection(tr, text, anchor, head, doInsert) { return doInsert ? tr.insertText(text) : tr.setSelection(TextSelection.create(tr.doc, anchor, head)) } // what we want to be able to do state.tr .chain(insertTextOrSetSelection, 'first', 1, 2, getDoInsert()) .insertText('second') // current workaround const tr = state.tr insertTextOrSetSelection(tr, 'first', 1, 2, getDoInsert()) tr.insertText('second') ``` You could imagine the branching logic within the custom function being a lot more complex, and maybe you'd even want to be able to chain multiple of them together. Since this is my first time contributing to prosemirror, I wasn't sure if we want features to have an associated ticket in the github issue tracker, or only bugs. If so, I'll create one and link it to this.

jschen5 (Migrated from github.com) reviewed

2020-01-09 02:03:04 +01:00

src/transaction.js

					
				@ -44,0 +44,4 @@

				  // :: ((Transaction, ...args: *)) → Transaction

				  // Applies the given function to the transaction. The function must

				  // take a [`Transaction`](#state.Transaction) as its first argument.

				  chain(transactionFunction, ...args) {

Not sure about this name. There's probably a better one🙂

jschen5 (Migrated from github.com) reviewed

2020-01-09 02:03:53 +01:00

src/transaction.js

					
				@ -41,6 +41,14 @@ export class Transaction extends Transform {

				    this.meta = Object.create(null)

				  }

				  // :: ((Transaction, ...args: *)) → Transaction

Wasn't sure if we needed to specify a return type of * for the argument function

Wasn't sure if we needed to specify a return type of `*` for the argument function

marijnh commented

2020-01-09 08:27:56 +01:00

(Migrated from github.com)

I don't think introducing a regular function call in a piece of code is a big enough problem to justify a feature like this. For things like conditionals and loops you'll already have to leave the method chaining format, you can make calls on a local variable for custom functions as well, I'd say.

jschen5 commented

2020-01-09 18:03:41 +01:00

(Migrated from github.com)

I'm probably misunderstanding what you're saying, but conditionals and loops are just what this would allow us to abstract away so that we could continue chaining. For example, you could do tr.chain(fn) where fn contains any number of loops or conditionals that tack on modifications to the transaction.

I'm probably misunderstanding what you're saying, but conditionals and loops are just what this would allow us to abstract away so that we could continue chaining. For example, you could do `tr.chain(fn)` where `fn` contains any number of loops or conditionals that tack on modifications to the transaction.

jschen5 commented

2020-01-09 21:46:28 +01:00

(Migrated from github.com)

Nevermind, I think I understand what you're saying. You're referring to the conditional within fn, which would not be chained, I guess. This new chain method is really just a way to abstract away this kind of complexity from the perspective of the caller, and so just to make sure I understand then, your opinion is that it's not worth adding this extra functionality to be able to create abstractions in these kinds of use cases?

Nevermind, I think I understand what you're saying. You're referring to the conditional within `fn`, which would not be chained, I guess. This new `chain` method is really just a way to abstract away this kind of complexity from the perspective of the caller, and so just to make sure I understand then, your opinion is that it's not worth adding this extra functionality to be able to create abstractions in these kinds of use cases?

marijnh commented

2020-01-10 14:04:33 +01:00

(Migrated from github.com)

Right, you could pass conditional or looping functions to chain, that's true. But I really don't think being able to chain everything is a very important thing for this interface to support, so no, I don't think this is worth adding.

Right, you could pass conditional or looping functions to `chain`, that's true. But I really don't think being able to chain everything is a very important thing for this interface to support, so no, I don't think this is worth adding.

👍 1

Pull request closed

This pull request cannot be reopened because the branch was deleted.

Sign in to join this conversation.

No reviewers

No labels

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

prosemirror/prosemirror-state!16

Rows
Columns