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

Editor.run tweaks #4067

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

Editor.run tweaks #4067

wants to merge 1 commit into from

Conversation

SomeHats
Copy link
Contributor

@SomeHats SomeHats commented Jul 3, 2024

Couple of changes to editor.run, the main one being the order. Previously, we did editor.run(fn, opts). This sort of makes sense, because options objects usually come last. This diff switched us to editor.run(opts, fn), which I think makes more sense for a few reasons:

  1. Callback-comes-last is IMO a stronger convention than options-come-last
  2. When the callback is long, having the options all the way down at the end and possibly off the screen makes it really hard to actually read what is happening and understand it. We know this code is it a run, but we don't know if that run is affecting history/locking until the bottom. having the options first makes things much more readable imo
  3. Better tooling support from e.g. formatters. This is related to 1, but the callback-comes-last convention is known by tools like prettier/biome and so they format it differently. For us, this means removing an unneeded layer of indentation, and keeping the run call and its options all on one line.

Other changes:

  • fix a bug where forced-ness wouldn't be restored properly after a run function
  • interface for run options for nicer docs
  • clearer name for ._force

@SomeHats SomeHats requested a review from steveruizok July 3, 2024 10:21
Copy link

vercel bot commented Jul 3, 2024

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

Name Status Preview Updated (UTC)
examples ✅ Ready (Inspect) Visit Preview Jul 3, 2024 10:21am
1 Skipped Deployment
Name Status Preview Updated (UTC)
tldraw-docs ⬜️ Ignored (Inspect) Jul 3, 2024 10:21am

@mimecuvalo
Copy link
Member

I'm not used to the options being in front, but seeing it coded up, it does make pause because it does seem more readable.

I think then my only concern is if the options is empty in the future, i.e. editor.run({}, () => {...doStuff...)
I could see this happening if we start moving towards the run function handling undo / redo more or other DX things like that. I would want wrapping my call in an editor.run() to:

  • handle any errors that come back and rollback appropriately (we had seen these kinds of issues recently)
  • handle undo/redo automagically for me
  • maybe other things

So, with that preface being said, and to riff on your idea further, Alex, could the options parameter by, ahem, optional? We can have an overloaded interface where it's: editor.run(optionsOrCallback: fn | {}, callback?: fn)

@SomeHats
Copy link
Contributor Author

SomeHats commented Jul 3, 2024

it is optional - there's a few examples in there that use plain editor.run(() => ...)

@mimecuvalo
Copy link
Member

it is optional - there's a few examples in there that use plain editor.run(() => ...)

do you mean in Steve's PR? yeah, i get that.
i'm just saying with your PR, it implies that the options are always front-loaded and they don't look optional. :)

@SomeHats
Copy link
Contributor Author

SomeHats commented Jul 3, 2024

no i mean here. it's already overloaded & optional:

run(opts: TLEditorRunOptions | undefined, fn: () => void): this
run(fn: () => void): this
run(...args: [opts: TLEditorRunOptions | undefined, fn: () => void] | [fn: () => void]): this {
let opts
let fn
if (args.length === 1) {
opts = undefined
fn = args[0]
} else {
opts = args[0]
fn = args[1]
}

@SomeHats
Copy link
Contributor Author

SomeHats commented Jul 3, 2024

i'll add an explicit example of that to the doc comment but the options-less signature is already there

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.

None yet

2 participants