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

client: Switch from jQuery.ajax to fetch #1215

Merged
merged 8 commits into from
Sep 11, 2020
Merged

client: Switch from jQuery.ajax to fetch #1215

merged 8 commits into from
Sep 11, 2020

Conversation

jtojnar
Copy link
Member

@jtojnar jtojnar commented Aug 28, 2020

This will bring us one step closer to removing jQuery.

Unfortunately the native promises lack built-in support for aborting so we need to somewhat clumsily return AbortController alongside them.

While at it, I also got rid of the jQuery Deferreds so the only remaining use of jQuery is the DOM manipulation. Hopefully, it will not remain for long.

@jtojnar jtojnar requested a review from niol August 28, 2020 06:57
@jtojnar jtojnar added this to the 2.19 milestone Aug 28, 2020
@jtojnar
Copy link
Member Author

jtojnar commented Sep 2, 2020

@niol will you be available to review this some time? I can also guide you through the Parcel-based build from #1137 if you need to bring up to speed.

@niol
Copy link
Collaborator

niol commented Sep 3, 2020

I should be able to look into it by tomorrow.

Copy link
Collaborator

@niol niol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cannot add a source, clicking does not show the form.

assets/js/selfoss-db.js Show resolved Hide resolved
@jtojnar
Copy link
Member Author

jtojnar commented Sep 4, 2020

Thanks for checking it out. It should be fixed now.

The add source button not functioning was caused by forgetting to call .text() on the fetch’s Response. I checked the rest of the code and found only two other places where we expect HTML instead of JSON and those called it correctly. Hopefully, I did not miss any.

I also had to fix the Sources\Write controller to handle missing tags field since the new field does not send it when there are no tags.

@niol
Copy link
Collaborator

niol commented Sep 5, 2020

With offline storage enabled, the client does not chain load items and stops after items_perpage.

@niol
Copy link
Collaborator

niol commented Sep 5, 2020

And then it seems to load the same entries over and over again.

@jtojnar jtojnar mentioned this pull request Sep 5, 2020
@niol
Copy link
Collaborator

niol commented Sep 5, 2020

I I shut down the server, it keeps the ajax requests unfinished although the "Connection refused" should terminate them immediately.

@jtojnar
Copy link
Member Author

jtojnar commented Sep 5, 2020

If I manually trigger selfoss.dbOffline.sendNewStatuses(), it seems to fetch another page. I am not sure what is supposed to trigger the fetch of the next page.

@niol
Copy link
Collaborator

niol commented Sep 5, 2020

when selfoss.dbOffline.newerEntriesMissing is true.

@jtojnar
Copy link
Member Author

jtojnar commented Sep 5, 2020

That seems to be true here but no luck.

@niol
Copy link
Collaborator

niol commented Sep 5, 2020

I would try the following but I'm stuck with a bundler error.

--- a/assets/js/selfoss-db.js
+++ b/assets/js/selfoss-db.js
@@ -157,16 +157,19 @@ selfoss.dbOnline = {
                     selfoss.dbOffline.storeEntries(data.newItems)
                         .then(function() {
                             selfoss.dbOffline.storeLastUpdate(dataDate);
-
                             selfoss.dbOnline._syncDone();
-
-                            // fetch more if server has more
-                            if (selfoss.dbOffline.newerEntriesMissing) {
-                                selfoss.dbOnline.sync();
-                            }
                         });
                 }
 
+                if (selfoss.dbOffline.newerEntriesMissing
+                    || selfoss.dbOffline.needsSync) {
+                    // There are still new items to fetch
+                    // or statuses to send
+                    syncing.request.promise.then(function () {
+                        selfoss.dbOffline.sendNewStatuses();
+                    });
+                }
+
                 if ('itemUpdates' in data) {
                     // refresh entry statuses in db and dequeue queued
                     // statuses but do not calculate stats as they are taken

@jtojnar
Copy link
Member Author

jtojnar commented Sep 6, 2020

Yeah, the bundlers are somewhat finicky in my experience. If I do git rebase with npm run dev running it breaks horribly, and I need to delete assets/.cache and restart it. OR sometimes on a syntax error, it is just swallowed and I need to restart it to see the error message. Unfortunately, when I tried Webpack it did not seem much better.

That seems to work with a minor tweak:

--- a/assets/js/selfoss-db.js
+++ b/assets/js/selfoss-db.js
@@ -159,14 +159,18 @@ selfoss.dbOnline = {
                             selfoss.dbOffline.storeLastUpdate(dataDate);
 
                             selfoss.dbOnline._syncDone();
-
-                            // fetch more if server has more
-                            if (selfoss.dbOffline.newerEntriesMissing) {
-                                selfoss.dbOnline.sync();
-                            }
                         });
                 }
 
+                if (selfoss.dbOffline.newerEntriesMissing
+                    || selfoss.dbOffline.needsSync) {
+                    // There are still new items to fetch
+                    // or statuses to send
+                    selfoss.dbOnline.syncing.request.promise.then(function () {
+                        selfoss.dbOffline.sendNewStatuses();
+                    });
+                }
+
                 if ('itemUpdates' in data) {
                     // refresh entry statuses in db and dequeue queued
                     // statuses but do not calculate stats as they are taken

Thanks.

@jtojnar
Copy link
Member Author

jtojnar commented Sep 6, 2020

But that patch triggers a sync loop you mention, the last page does not seem to get picked up:

GET http://localhost/selfoss/items/sync?since=2020-09-05T15%3A53%3A06.000Z&tags=true&itemsStatuses=true&itemsSinceId=696&itemsNotBefore=2020-09-05T03%3A06%3A45.277Z&itemsHowMany=51
key value
lastUpdate "2020-09-05 17:53:06"
newItems [ {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, … ]
0 Object { id: 88, datetime: "2020-08-05T16:02:52+02:00", unread: true, … }
1 Object { id: 478, datetime: "2020-08-27T23:00:17+02:00", unread: false, … }
2 Object { id: 631, datetime: "2020-08-27T19:00:03+02:00", unread: true, … }
3 Object { id: 648, datetime: "2020-09-04T09:31:00+02:00", unread: true, … }
4 Object { id: 649, datetime: "2020-08-28T11:19:00+02:00", unread: true, … }
5 Object { id: 650, datetime: "2020-08-27T10:00:00+02:00", unread: true, … }
6 Object { id: 651, datetime: "2020-08-27T08:57:00+02:00", unread: true, … }
7 Object { id: 653, datetime: "2020-08-19T12:45:00+02:00", unread: true, … }
8 Object { id: 654, datetime: "2020-08-06T12:40:00+02:00", unread: true, … }
9 Object { id: 655, datetime: "2020-07-29T07:36:00+02:00", unread: true, … }
10 Object { id: 656, datetime: "2020-07-29T07:22:00+02:00", unread: true, … }
11 Object { id: 657, datetime: "2020-07-28T15:57:00+02:00", unread: true, … }
12 Object { id: 658, datetime: "2020-07-27T09:30:00+02:00", unread: true, … }
13 Object { id: 659, datetime: "2020-07-21T09:55:00+02:00", unread: true, … }
14 Object { id: 660, datetime: "2020-07-17T07:52:00+02:00", unread: true, … }
15 Object { id: 661, datetime: "2020-07-10T17:20:00+02:00", unread: true, … }
16 Object { id: 662, datetime: "2020-07-09T17:17:00+02:00", unread: true, … }
17 Object { id: 663, datetime: "2020-06-24T14:59:00+02:00", unread: true, … }
18 Object { id: 664, datetime: "2020-06-24T12:29:00+02:00", unread: true, … }
19 Object { id: 665, datetime: "2020-06-24T11:52:00+02:00", unread: true, … }
20 Object { id: 666, datetime: "2020-06-22T07:47:00+02:00", unread: true, … }
21 Object { id: 667, datetime: "2020-06-18T13:53:00+02:00", unread: true, … }
22 Object { id: 668, datetime: "2020-06-10T07:36:00+02:00", unread: true, … }
23 Object { id: 669, datetime: "2020-05-15T12:20:00+02:00", unread: true, … }
24 Object { id: 670, datetime: "2020-05-15T08:16:00+02:00", unread: true, … }
25 Object { id: 671, datetime: "2020-05-14T09:23:00+02:00", unread: true, … }
26 Object { id: 672, datetime: "2020-05-11T08:31:00+02:00", unread: true, … }
27 Object { id: 673, datetime: "2020-05-08T16:34:00+02:00", unread: true, … }
28 Object { id: 674, datetime: "2020-05-06T15:11:00+02:00", unread: true, … }
29 Object { id: 675, datetime: "2020-04-30T15:57:00+02:00", unread: true, … }
30 Object { id: 676, datetime: "2020-04-30T10:38:00+02:00", unread: true, … }
31 Object { id: 677, datetime: "2020-04-21T08:42:00+02:00", unread: true, … }
32 Object { id: 678, datetime: "2020-04-12T15:05:00+02:00", unread: true, … }
33 Object { id: 679, datetime: "2020-04-03T12:02:00+02:00", unread: true, … }
34 Object { id: 680, datetime: "2020-04-01T12:35:00+02:00", unread: true, … }
35 Object { id: 681, datetime: "2020-03-17T08:45:00+01:00", unread: true, … }
36 Object { id: 682, datetime: "2020-03-02T16:12:00+01:00", unread: true, … }
37 Object { id: 683, datetime: "2020-02-26T09:25:00+01:00", unread: true, … }
38 Object { id: 684, datetime: "2020-02-19T07:12:00+01:00", unread: true, … }
39 Object { id: 685, datetime: "2020-02-14T09:02:00+01:00", unread: true, … }
40 Object { id: 686, datetime: "2020-02-14T07:00:00+01:00", unread: true, … }
41 Object { id: 687, datetime: "2020-02-13T09:30:00+01:00", unread: true, … }
42 Object { id: 688, datetime: "2020-09-04T17:36:24+02:00", unread: true, … }
43 Object { id: 689, datetime: "2020-09-04T17:32:50+02:00", unread: true, … }
44 Object { id: 690, datetime: "2020-08-19T16:01:39+02:00", unread: true, … }
45 Object { id: 691, datetime: "2020-09-03T08:19:17+02:00", unread: true, … }
46 Object { id: 692, datetime: "2020-09-03T08:19:12+02:00", unread: true, … }
47 Object { id: 693, datetime: "2020-09-03T08:19:06+02:00", unread: true, … }
48 Object { id: 694, datetime: "2020-09-03T08:18:59+02:00", unread: true, … }
49 Object { id: 695, datetime: "2020-07-21T12:13:14+02:00", unread: true, … }
50 Object { id: 696, datetime: "2020-07-13T19:00:37+02:00", unread: true, … }
lastId 754

@niol
Copy link
Collaborator

niol commented Sep 7, 2020

The sync loop seems to be specific to sqlite and how we compare dates as simple strings.

We should either use specific syntax for sqlite queries or ensure compared dates share the same timezone.

For instance, the following seems to be fixing the problem.

--- a/src/controllers/Items/Sync.php
+++ b/src/controllers/Items/Sync.php
@@ -56,11 +56,12 @@ class Sync {
         }

         $since = new \DateTime($params['since']);
+        $since->setTimeZone(new \DateTimeZone(date_default_timezone_get()));

         $last_update = new \DateTime($this->itemsDao->lastUpdate());

         $sync = [
-            'lastUpdate' => $last_update->format('Y-m-d H:i:s'),
+            'lastUpdate' => $last_update->format(\DateTime::ATOM),
         ];

         $sinceId = 0;
@@ -73,6 +74,7 @@ class Sync {
                     // only send 1 day worth of items
                     $notBefore = new \DateTime();
                     $notBefore->sub(new \DateInterval('P1D'));
+                    $notBefore->->setTimeZone(new \DateTimeZone(date_default_timezone_get()));
                 }

                 $itemsHowMany = $f3->get('items_perpage');

assets/js/selfoss-db.js Outdated Show resolved Hide resolved
jtojnar and others added 7 commits September 11, 2020 15:55
This will bring us one step closer to removing jQuery.

Unfortunately the native promises lack built-in support for aborting so we need to clumsily return AbortController alongside them.
So that `then` handlers only fire on success and we can attach `catch` handlers to it.
@jtojnar jtojnar merged commit 4ecb212 into master Sep 11, 2020
@jtojnar jtojnar deleted the fetch branch September 11, 2020 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants