search

remotestorage / remotestorage-widget

⬡ Connect widget for remoteStorage.js
https://remotestorage.io
28 stars 16 forks source link

Widget should not handle SyncError event #92

Closed iLiviu closed 4 years ago

iLiviu commented 5 years ago

The update to sync.js on RemoteStorage library that fixed a bug on emitting a SyncError when sync fails, revealed a problem in the widget when this error is emitted. The widget's state turns to error and it displays the error text, replacing the offline state, which was set right before this error (the SyncError event is emitted when the backend rejects it's promise on a HTTP request due to network error or timeout, and when this happens a network-offline event is also emitted, which widget catches and sets appropriate state). Because the error state is set, the widget won't return to it's connected state even when app comes back online. So i think setting offline state is enough, and SyncError should just be ignored.

raucao commented 5 years ago

Hmm. This one is a bit weird. I was almost certain that normal failed HTTP requests should not throw a SyncError, but that those errors are actual real issues with the sync itself. IIRC the reason the widget does this is so that you have to disconnect and with that clear the cache, so that the re-sync can work.

@galfert @michielbdejong Do you remember anything about this perhaps? I feel like this might have been changed accidentally when we (optionally) switched from old-school AJAX to fetch(). But haven't looked into it yet.

galfert commented 5 years ago

I also don't think it should emit a SyncError event, but I'm not sure either.

iLiviu commented 5 years ago

From what i see in the code, the intention is clearly to be emitted on timeout or failed HTTP request. For real issues (unexpected HTTP responses) a generic Error is emitted instead.

DougReeder commented 5 years ago

It might be of value go through the unit tests and see if these cases are properly tested. I reworked the lowest level of unit tests to be run against both XHR and fetch(), but not the unit tests for the higher levels.

On Feb 25, 2019, at 9:48 AM, Liviu Iancuta notifications@github.com wrote:

From what i see in the code, the intention is clearly to be emitted on timeout or failed HTTP request. For real issues (unexpected HTTP responses) a generic Error is emitted instead.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

Doug Reeder reeder.29@gmail.com Recall your notes by context, with Serene Notes: https://serenenotes.hominidsoftware.com/ My gender pronouns are: he, him, his.