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

Remove load API #46

Open
gterzian opened this issue Sep 8, 2023 · 4 comments
Open

Remove load API #46

gterzian opened this issue Sep 8, 2023 · 4 comments

Comments

@gterzian
Copy link
Collaborator

gterzian commented Sep 8, 2023

I propose we remove the load API, because:

  1. It makes the internal state machine much more complicated.
  2. I believe it is unnecessary because if one knows that the document is available locally, and there are no network connections open, then request will have the same effect as load.
@gterzian
Copy link
Collaborator Author

gterzian commented Sep 8, 2023

cc @alexjg

@alexjg
Copy link
Collaborator

alexjg commented Sep 8, 2023

My main thought is what if you dont know the document is available locally but you don't want to wait for the network for some reason (e.g. you are writing an application which expects to be on a high latency network and you want to display a pop up telling the user that you didn't have it locally and so you're asking the network).

One alternative way to achieve this is the way the JS implementation does it, by returning a handle from request immediately and exposing some kind of status field on the handle, but I don't think that simplifies the state machine at all right?

@gterzian
Copy link
Collaborator Author

gterzian commented Sep 11, 2023

you dont know the document is available locally but you don't want to wait for the network for some reason

How about the following:

  1. Add a local_load_only boolean argument to request_document.
  2. When the storage future resolves, if it is Ok(None) and local_load_only was true, then resolve the request with an error(or with Ok(None))).

That way at that point an app embedding Spanreed could show the pop up, and ask the user whether to continue with a network request(by doing a second request_document with a false local_load_only).

This would simplify the state machine, as the above can be implemented by:

  1. Add the local_load_only to DocState::Bootstrap.
  2. Add a conditional here to implement 2 above.
  3. Remove everything related to DocState::LoadPending

In the scenario sketched above, the second request_document with the flag set to false can still just go through the local load workflow, so nothing needs to change there.

Deterministic testing should be easy too, to the point where this could be marked as "good first issue" for an outside contributor.

@RustLoverTheCoder
Copy link

What I'm wondering about the load api is, why does the load doc get an error the second time
My understanding is to request the document again

@alexjg

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

No branches or pull requests

3 participants