rocicorp / repc

The canonical Replicache client, implemented in Rust.
Other
31 stars 7 forks source link

Need to signal scan errors to bindings #263

Closed arv closed 3 years ago

arv commented 3 years ago

My point is that we should throw, or tell the bindings about the error some other way. Just logging the error leads to invalid state.

Originally posted by @arv in https://github.com/rocicorp/repc/pull/261#r520843397

phritz commented 3 years ago

My point is that we should throw, or tell the bindings about the error some other way. Just logging the error leads to invalid state.

agree we should pass the error along and let the bindings know.

what will the bindings do with it? this i guess is a general question: how do the bindings treat repc errors? i guess the answer probably depends on which errors we are talking about:

arv commented 3 years ago

dispatch returns a Result. Results that are in Err state throw their Err JsValue (this is handled by wasm_bindgen). This covers most of the cases.

arv commented 3 years ago

For scan receiver just pushes the result to an array. But if there are errors that array will have the wrong length and we end up with an iterator that yields undefined for entries() / values() / keys()

phritz commented 3 years ago

does it yield undefined for all the entries or just those that errored? what im getting at i guess is i'm wondering how much we leave up to the app (return the errors but also any good results we get) vs decide for them (blow up if we hit any error)?

arv commented 3 years ago

I think I'm wrong. Things will just work:

https://github.com/rocicorp/replicache-sdk-js/blob/3fa513d41834f9292edf06a270319c73a78b0df0/src/scan-iterator.ts#L70-L75

We only push elements to _scanItems in the receiver callback. That means that we get less items back than the limit passed in. Not a big deal.

(Also, saw this now; The check this._current >= this._limit is no longer needed)