Change Store.save() to return a Promise

[texuf]

The only implementation of this is an async function, but I can’t await it because the interface hides the return type.

signed-off-by austin ellis austin@hntlabs.com

Type: defect

---

Here's what your changelog entry will look like:

🚨 BREAKING CHANGES

• Change Store.save() to return a Promise (#3221). Contributed by @texuf.

[richvdh]

@texuf thanks for your contribution!

I think I'm missing some context on this. Is there a particular problem you are trying to solve?

I don't think IStore.save is meant to be used from outside the js-sdk (though I could be wrong on this; it's not an area I'm terribly familiar with). My understanding is that it is automatically called from sync.ts as part of the /sync loop, so I'm curious as to your usecase.

I'm also not sure that it matters that we don't wait for the promise to complete - there's not much we can do with the knowledge in any case.

[richvdh]

@t3chguy I see you've labelled this as a breaking change, but we're changing a method that returned void into one which returns something else (a Promise, in this case). I don't think there's any way that can break existing application code unless someone was inexplicably relying on save to return undefined?

[t3chguy]

@richvdh typescript strict mode can get unhappy about this scenario

[texuf]

@texuf thanks for your contribution!

I think I'm missing some context on this. Is there a particular problem you are trying to solve?

I don't think IStore.save is meant to be used from outside the js-sdk (though I could be wrong on this; it's not an area I'm terribly familiar with). My understanding is that it is automatically called from sync.ts as part of the /sync loop, so I'm curious as to your usecase.

I'm also not sure that it matters that we don't wait for the promise to complete - there's not much we can do with the knowledge in any case.

I am writing tests where I'd like to verify that the saved sync interacts as I would expect with my application code.
My tests do the following

• matrixClient.store.save(true) // necessary because the store saves periodically, I would like to force a write
• matrixClient.stopClient()
• matrixClient = undefined

Then I go on to create a new matrix client with the same credentials (simulating a browser refresh) and verify that the saved sync is loaded and correctly populates my ui

If I don't wait for the save to complete, I have unreliable tests because I don't know when the async operation has completed.

I think there will be instances in the future where I will want to ensure data is persisted before performing a user action, but that is speculation, the main use case is for writing reliable tests.

[richvdh]

Seems fair. Thanks for the explanation!

[texuf]

Can anyone help me with this downstream test error?

0s
Run exit [1](https://github.com/matrix-org/matrix-js-sdk/actions/runs/4513595824/jobs/7949013225#step:2:1)
  exit 1
  shell: /usr/bin/bash -e {0}
Error: Process completed with exit code [1](https://github.com/matrix-org/matrix-js-sdk/actions/runs/4513595824/jobs/7949013225#step:3:1).

not sure what do here.

[texuf]

Can anyone help me with this downstream test error?

0s
Run exit [1](https://github.com/matrix-org/matrix-js-sdk/actions/runs/4513595824/jobs/7949013225#step:2:1)
  exit 1
  shell: /usr/bin/bash -e {0}
Error: Process completed with exit code [1](https://github.com/matrix-org/matrix-js-sdk/actions/runs/4513595824/jobs/7949013225#step:3:1).

not sure what do here.

Ah i see, a previous test failed, still not sure if this is on me

Error: expect(element).toHaveAttribute("contentEditable", "true") // element.getAttribute("contentEditable") === "true"

Expected the element to have attribute:
  contentEditable="true"
Received:
  contentEditable="false"

[richvdh]

Yeah, I don't understand this either. I've asked for help from the rest of the team but it's Friday evening now.

[t3chguy]

Looks like a WYSIWYG test is flaky

[texuf]

@t3chguy do you think we could try the merge queue again?

[richvdh]

This does seem to be causing an actual failure, unfortunately

[richvdh]

possibly the problem is that we are now waiting for IndexedDBStore.save() to complete, whereas previously it would still fail, but scream its errors into the void?

[texuf]

possibly the problem is that we are now waiting for IndexedDBStore.save() to complete, whereas previously it would still fail, but scream its errors into the void?

Thanks for looking into it. I will see if I can figure it out later tonight.

[texuf]

I don't see much difference between my run and a successful one, the output is pretty rough to look at. I created a PR for the only thing I could find: element-hq/element-web#25061

[richvdh]

the output is pretty rough to look at.

It absolutely is. I think this is mostly a result of our jest configuration; #3269 should improve that for the js-sdk, and I'll replicate it for the other projects soon. (Though also, the failing test uses matrix-mock-request, which is... crap.)

However, on closer inspection of the logs from the failed run, it seems like the failure mode is already known, so I'm going to mash the merge button again. Sorry for the run-around on this.