Skip to content

Change Pane::saveItems to return a promise… - #1596

Open
savetheclocktower wants to merge 2 commits into
masterfrom
asyncify-pane-saveitems
Open

Change Pane::saveItems to return a promise…#1596
savetheclocktower wants to merge 2 commits into
masterfrom
asyncify-pane-saveitems

Conversation

@savetheclocktower

@savetheclocktower savetheclocktower commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

…that resolves when all items have been saved.

I was doing some tending of the types repo and looking over our documentation for Pane. I realized that nearly all of our pane methods related to saving items returned promises — Pane::saveItem, Pane::saveActiveItem, Pane::saveItemAs, Pane::saveActiveItemAs. This reflects the fact that saving can go async for a couple of reasons:

  • First, we might need to prompt the user if the item is conflicted to see how they want to handle the conflict.
  • The save method on a pane item anticipates that saving itself might be an async process. (For instance: anything can attach an async onWillSave callback to a TextBuffer; when TextBuffer::save is called, it does not resolve until all those async callbacks resolve.)

So it's weird to me that Pane::saveItems doesn't go async. When called, it finds all the items that need saving in a given pane (the ones for whom isModified is defined and returns true) and then calls Pane::saveItem on each in turn. But then it just returns!

I don't have a specific use case in mind, but it took me all of 15 minutes to enhance Pane::saveItems so that it goes async and doesn't resolve until all modified items in the pane have finished saving. This should have no effect on existing usages of saveItems, since that function did not have any return value previously.

Testing

I added a new spec in the Pane suite; if CI likes it, that should be good enough to prove that this works.

The spec was added in isolation in the first commit of this PR; so if you like, you can check out that commit and run the new spec in order to verify that it fails. Then you can check out the second commit to verify that the new spec passes.

@savetheclocktower savetheclocktower added this to the 1.133.0 milestone Jul 18, 2026
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.

1 participant