Expand out offline app features for smoother cache clearing and version tracking #1384

anthony-zhou · 2019-12-22T02:05:44Z

So far, I've displayed the NPM version number at the bottom of the page by parsing it from the package.json file. I'm working on also adding the version number to the top right and detecting when a new version has successfully been fetched.

tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with npm test
code is in uniquely-named feature branch and has no merge conflicts
PR is descriptively titled
Insert-step functionality is working correct as expected.

Screenshots

Here is a gif of the version number text, in the top right and at the bottom of the page:

Here is a screenshot of the prompt to update to the latest version (in the top left):

Update from image sequencer master

codecov · 2019-12-22T02:09:56Z

Codecov Report

Merging #1384 into main will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1384   +/-   ##
=======================================
  Coverage   66.35%   66.35%           
=======================================
  Files         125      125           
  Lines        2574     2574           
  Branches      406      406           
=======================================
  Hits         1708     1708           
  Misses        866      866

harshkhandeparkar

Your fix will display the current version number but won't show the latest version number without clearing the cache.

anthony-zhou · 2019-12-23T03:54:01Z

@harshkhandeparkar I'm working on the feature you mentioned, but as part of this I'd like to use the Node module semver. I'm currently working in demo.js. For some reason the traditional const semver = require('semver') is throwing a Javascript error of

image-sequencer.js:1 Uncaught Error: Cannot find module './functions/lt'
    at o (image-sequencer.js:1)
    at o (image-sequencer-ui.js:1)
    at o (image-sequencer-ui.js:1)
    at image-sequencer-ui.js:1
    at lazyRequire (image-sequencer-ui.js:47990)
    at Object.get [as lt] (image-sequencer-ui.js:47997)
    at XMLHttpRequest.request.onreadystatechange (image-sequencer-ui.js:48)```

Would you happen to know what is causing this error?

keshav234156 · 2019-12-23T08:39:15Z

@anthony-zhou Do you know Service Workers? It is basically a script that browsers in run in the background that enable to control network request and cache those requests. Each service Worker has a lifecycle namely registration, Installation and Activation and installation is triggered only if you visit the application for the first time or there is some difference in the new service worker. So we can add event listeners to the service worker. Then we can check whether service worker was installed or not and do all the required stuff.
Thanks

harshkhandeparkar · 2019-12-23T17:18:06Z

examples/demo.js

+        }
+      }
+    }
+    request.open("GET", "https://api.github.com/repos/publiclab/image-sequencer/contents/package.json", true);


This is cool. So you want to clear the cache(via demo.js) if a new version is available?

Yeah, but I'll probably end up doing this through the service worker.

This is not a bad way either. Is there a downside to this?

Fetching the latest and local version number is now done through versionManagement.js

A new version is available whenever the sw.js file is changed.

anthony-zhou · 2019-12-28T21:47:42Z

I made some more changes:

Moved fetching of local and latest version number to versionManagement.js.
Added a popup in index.html that is displayed when a new service worker is loaded.
This works by loading a new service worker when sw.js is changed.

Currently, the popup I created is displayed whenever the sw.js file is changed, which means that when a new version of Image Sequencer is deployed we would also need to change the version number in the first line of sw.js. Then we could compare the local version number to the online version number and get the new version if needed.

I'm thinking it might be inconvenient to update sw.js each time a new version is deployed. Is there any way we could do this automatically? @harshkhandeparkar @keshav234156

harshkhandeparkar · 2019-12-29T06:54:06Z

I don't think doing it automatically will be easy. We can't browserify it. We will either have to make a script which changes the specific line in the code or let sw.js fetch changes from the package.json file.

anthony-zhou · 2019-12-29T18:49:07Z

I just pushed a commit that adds a faint version number in fixed position in the top right corner.

So now, I believe the app should fetch the newest version whenever a new version is deployed, as long as sw.js is changed when we deploy a new version. So the changes technically work now--the question is whether we need to implement automatic updates to sw.js. This would make deploying a new version easier but would require running an additional script each time changes are made to package.json.

In order to update sw.js automatically, I think the best option may be to run a script that changes the sw.js file whenever package.json is changed. However, this would probably require editing some of the configuration files here on GitHub. Does it make sense to add a script in GitHub that does this (presumably it would have to be run whenever changes are made to the repository to check if changes were made to package.json)?

Changed z-index

Used grunt-text-replace

anthony-zhou · 2019-12-30T21:06:16Z

@keshav234156 @harshkhandeparkar I ended up creating a grunt task to update sw.js whenever grunt build is run. I used the dependency grunt-text-replace to replace the text for the version number. Now everything should be working as expected. I would appreciate if you could review this PR again. Thanks!

Gruntfile.js

harshkhandeparkar · 2019-12-31T12:06:08Z

examples/lib/versionManagement.js

+      callback(latestVersionNumber);
+    }
+  }
+  request.open("GET", "https://api.github.com/repos/publiclab/image-sequencer/contents/package.json", true);


This will GET the latest package JSON. It may not be the one online. We need a better way of GET-ing the version number.

By online, do you mean at sequencer.publiclab.org? If so, it looks like we could get the version number from the stable branch here on GitHub.

Whether we do this depends on whether offline users are mainly working off the stable branch or the main branch. The current implementation of this function is getting the version number from the main branch, which may be what people are using offline anyways.

For beta.sequencer.publiclab.org, we'll have to get the version from that repo. Beta is on a separate repo btw. For the stable one, it will bw different. Anyway, it will be a mess. Can't we get it from current_location/package.json or something? Make it dynamic instead of hard coded?

harshkhandeparkar · 2019-12-31T12:07:12Z

examples/lib/versionManagement.js

+  return v1parts.length != v2parts.length;
+}
+
+function compare(part1, part2) {


The parts of a version number are called major.minor.patch. Could you please use that terminology here too? To make it more readable.

harshkhandeparkar · 2019-12-31T12:07:35Z

package.json

@@ -70,6 +70,7 @@
    "readline-sync": "^1.4.7",
    "save-pixels": "~2.3.4",
    "selectize": "^0.12.6",
+    "semver": "^7.1.1",


Is semver used now?

Good point. I have now uninstalled it.

examples/lib/versionManagement.js

Make version statements more descriptive.

Remove unused versionCompare function

anthony-zhou · 2019-12-31T22:07:57Z

@harshkhandeparkar Thanks for all the feedback! I removed the versionCompare() function in versionManagement.js, which wasn't being used anyways and was responsible for most of the messy code in that file, so hopefully that addresses several of your comments. Please see my individual replies for more details.

harshkhandeparkar · 2020-01-01T14:02:03Z

examples/lib/versionManagement.js

+      callback(latestVersionNumber);
+    }
+  }
+  request.open("GET", "https://api.github.com/repos/publiclab/image-sequencer/contents/package.json", true);


For beta.sequencer.publiclab.org, we'll have to get the version from that repo. Beta is on a separate repo btw. For the stable one, it will bw different. Anyway, it will be a mess. Can't we get it from current_location/package.json or something? Make it dynamic instead of hard coded?

keshav234156 · 2020-01-06T19:55:32Z

@jywarren CAn you please review this ASAP

jywarren · 2020-01-06T20:04:25Z

OK, thanks for your patience! I'll review this next!

jywarren · 2020-01-06T21:08:31Z

OK, my understanding of getLatestVersionNumber() was that we were going to use it to establish whether https://sequencer.publiclab.org (or beta, but let's put that aside for now for simplicity) has a newer version available than what's been cached. So it should compare:

the npm version of the cached app TO
the npm version of the app available at sequencer.publiclab.org

Is that right? OK, now I'll look through the latest files to see how this connects together. Thanks!

jywarren · 2020-01-06T21:11:18Z

examples/lib/cache.js

  if ('serviceWorker' in navigator) {
    navigator.serviceWorker.register('sw.js', { scope: '/examples/' })
      .then(function(registration) {
+        registration.addEventListener('updatefound', () => {


So, here we can detect if there's a new version, but we don't actually tie this to getLatestVersionNumber(), as far as I can read it? How might we do that - could we say Click to install version X here? Or is getLatestVersionNumber() not reading from the same source as updatefound here?

We don't have to change things; i like how this looks. But let's leave enough inline comments to make the answer to this question really clear! Esp. because this is complicated stuff, so future coders should be able to follow what you've done. Thanks!

jywarren

OK great, we can get this merge-able if we add explanatory comments at least, or if you'd like to link updatefound to getLatestVersionNumber() more explicitly we could also do that. Thank you!

jywarren · 2020-01-06T21:13:48Z

And thanks for your patience! Next time, perhaps we can work in smaller chunks so that it can be merged in a modular fashion, and we won't get so slowed down, as it seems this PR has several portions which are already complete and could be merged on their own!

Thanks, all!

anthony-zhou · 2020-01-06T22:22:50Z

@jywarren Thanks for the feedback. I added some explanatory comments to versionManagement.js , cache.js and sw.js in the areas I worked on. Is there anything else to add?

Also, good suggestion about working in a modular fashion... I'll be sure to do that next time.

examples/lib/versionManagement.js

…into main

jywarren · 2020-01-07T16:35:46Z

Oh no! Tests didn't pass this time. Was there a typo perhaps?

harshkhandeparkar

Sorry for the late review. Only a few changes. Everything else looks fine to me.

examples/index.html

harshkhandeparkar · 2020-01-07T16:52:27Z

examples/lib/cache.js

+    $('#update-prompt-modal').addClass('show');
+  }
+
+  // When a new service worker has been loaded, the button in the update prompt


Please use multi-line comments instead of multiple inline comments. That will make it easier to read. Also, please keep words like service and worker on the same line.

Got it, thanks!

harshkhandeparkar · 2020-01-07T16:53:12Z

examples/lib/versionManagement.js

+ */
+function getLatestVersionNumber(callback) {
+  // Get the homepage reference from the local package.json.
+  var homepage = require('../../package.json').homepage;


examples/lib/versionManagement.js

harshkhandeparkar · 2020-01-07T16:55:07Z

examples/lib/versionManagement.js

+      var latestVersionNumber = response.version;
+
+      // Do something with the version number using a callback function.
+      callback(latestVersionNumber);


If it is optional then please add an

if (callback)

harshkhandeparkar · 2020-01-07T16:55:44Z

examples/lib/versionManagement.js

+
+// Get the version number from the local package.json file.
+function getLocalVersionNumber() {
+  return require('../../package.json').version;


Why does the require statement appear twice?

Also, I think adding all the require statements at the start of the file is a good practice.

I moved the require statement to the start of the file, so it's now called only once.

examples/sw.js

harshkhandeparkar · 2020-01-07T16:59:02Z

I think travis is broken. It had broken before. It could fix itself like before. Perhaps the servers are down or there are too many requests.

jywarren · 2020-01-07T17:02:47Z

It's possible Travis is down... but looks ok here; https://www.traviscistatus.com/

jywarren · 2020-01-07T17:03:24Z

Oh it passed now! cool. But it's out of sync. I'll sync it.

jywarren · 2020-01-07T17:25:30Z

Just waiting on some answers to @harshkhandeparkar's questions, then ready to merge this!

Changed single-line comments to multiline comments.

anthony-zhou · 2020-01-07T17:37:47Z

@jywarren @harshkhandeparkar Thanks for reviewing. I have implemented all the changes you mentioned.

jywarren · 2020-01-07T17:48:21Z

Fantastic. Waiting for this to pass before merging. Thanks a ton, @anthony-zhou !!!

jywarren · 2020-01-07T19:56:44Z

Whew!!! Great!!!!! Thanks a ton!

anthony-zhou and others added 2 commits December 21, 2019 19:03

Merge pull request #1 from publiclab/main

9cece82

Update from image sequencer master

Add version number to bottom

392149b

harshkhandeparkar requested changes Dec 22, 2019

View reviewed changes

Get latest version number from GitHub

1d165d6

harshkhandeparkar reviewed Dec 23, 2019

View reviewed changes

anthony-zhou added 2 commits December 28, 2019 15:38

Create versionManagement.js

c03f981

Fetching the latest and local version number is now done through versionManagement.js

Add popup to prompt for refresh when a new version is available

526f2de

A new version is available whenever the sw.js file is changed.

Add version number fixed in top right corner

20aac76

anthony-zhou added 6 commits December 30, 2019 11:12

Fix Codeclimate issues

27c7c97

Update versionManagement.js

6a6b408

Update versionManagement.js

f97411e

Make update prompt appear at front of page

56003b3

Changed z-index

Delete unecessary code

e5c1faf

Create task to automatically update sw.js

64378a6

Used grunt-text-replace

anthony-zhou changed the title ~~[WIP] Expand out offline app features for smoother cache clearing and version tracking~~ Expand out offline app features for smoother cache clearing and version tracking Dec 30, 2019

harshkhandeparkar requested changes Dec 31, 2019

View reviewed changes

anthony-zhou added 4 commits December 31, 2019 15:39

Uninstall semver

b0e9ff6

Add replace task to serve and production tasks

6b9d16c

Update demo.js

124ff32

Make version statements more descriptive.

Update versionManagement.js

c294505

Remove unused versionCompare function

anthony-zhou requested a review from harshkhandeparkar December 31, 2019 22:08

harshkhandeparkar requested changes Jan 1, 2020

View reviewed changes

Merge branch 'main' into main

01e9882

jywarren reviewed Jan 6, 2020

View reviewed changes

jywarren requested changes Jan 6, 2020

View reviewed changes

anthony-zhou added 2 commits January 6, 2020 16:12

Added explanatory comments

47b3a39

Update versionManagement.js

839f637

Merge branch 'main' into main

ef337b8

jywarren reviewed Jan 6, 2020

View reviewed changes

examples/lib/versionManagement.js Show resolved Hide resolved

anthony-zhou added 2 commits January 7, 2020 09:48

Update versionManagement.js

0b34074

Merge branch 'main' of https://github.com/anthony-zhou/image-sequencer …

384d526

…into main

Merge branch 'main' into main

3e10399

harshkhandeparkar requested changes Jan 7, 2020

View reviewed changes

Merge branch 'main' into main

3d54f41

anthony-zhou added 3 commits January 7, 2020 11:31

Updates for readability

a4f0b43

Changed single-line comments to multiline comments.

Update versionManagement.js

405a3b0

Update versionManagement.js

c5b8fc2

harshkhandeparkar approved these changes Jan 7, 2020

View reviewed changes

jywarren merged commit 00ed0f1 into publiclab:main Jan 7, 2020

Expand out offline app features for smoother cache clearing and version tracking #1384

Expand out offline app features for smoother cache clearing and version tracking #1384

Conversation

anthony-zhou commented Dec 22, 2019 • edited

Screenshots

codecov bot commented Dec 22, 2019 • edited

Codecov Report

harshkhandeparkar left a comment

Choose a reason for hiding this comment

anthony-zhou commented Dec 23, 2019 • edited

keshav234156 commented Dec 23, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anthony-zhou commented Dec 28, 2019

harshkhandeparkar commented Dec 29, 2019

anthony-zhou commented Dec 29, 2019

anthony-zhou commented Dec 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anthony-zhou commented Dec 31, 2019

Choose a reason for hiding this comment

keshav234156 commented Jan 6, 2020

jywarren commented Jan 6, 2020

jywarren commented Jan 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jywarren left a comment

Choose a reason for hiding this comment

jywarren commented Jan 6, 2020

anthony-zhou commented Jan 6, 2020

jywarren commented Jan 7, 2020

harshkhandeparkar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harshkhandeparkar commented Jan 7, 2020

jywarren commented Jan 7, 2020

jywarren commented Jan 7, 2020

jywarren commented Jan 7, 2020

anthony-zhou commented Jan 7, 2020

jywarren commented Jan 7, 2020

jywarren commented Jan 7, 2020

anthony-zhou commented Dec 22, 2019 •

edited

codecov bot commented Dec 22, 2019 •

edited

anthony-zhou commented Dec 23, 2019 •

edited