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

Express migration + website caching #26

Merged
merged 57 commits into from
Jul 1, 2014

Conversation

brianhanifin
Copy link
Contributor

I am recommending this code be merged with the main branch immediately. We have discussed it a lot on the dev group. I just tested a javascript alert in client.js, which was immediately displayed on the first reload (reload button only, no shift required). I also changed the text and the change was again displayed on the first reload.

This tells me the new caching mechanism is recognizing when the files have changed. I believe this proves our code changes will be recognized immediately.

brianhanifin and others added 30 commits June 20, 2014 18:55
Merge retrospective-prediction
Thanks to advice by Adrien de Croy I added/updated the following sample
headers.

cache-control: public, max-age=2592000
expires: Thu Jul 24 2014 17:16:14 GMT-0700 (PDT)
Arr-Disable-Session-Affinity: True
Upgraded from node-static to Express, and now the site caches like a
champ!
Upgraded from node-static to express.
and now the site caches like a
champ!
Tested by deleting everything, running npm install from scratch.
This helps ensure that only the files in the static subdirectory can be served.
Should eliminate serving config files via http.
move static files into /static
and other dynamic elements.
This isn’t used at the current time.
Added “dateformat” package. Plus some code reorganization.
Added “dateformat” package. Plus some code reorganization.
Added “dateformat” package. Plus some code reorganization.
Added “dateformat” package. Plus some code reorganization.
Adding socket.io.js to nightscout.appcache, plus a little cleanup.
to only include audio, favicon, and google fonts. HTTP Header Cache
caches all.
to only include audio, favicon, and google fonts. HTTP Header Cache
still caches all.
so we can utilize SSL without breaking the appearance of the site.
brianhanifin and others added 8 commits June 30, 2014 14:01
Caching these files this aggressively is trouble. :)
Fix now-line placement to be sensitive to when panning is or isn't
being used.

This allows fixes a problem where the now-line was off by ten minutes.
now testing on azure
on azure
on both Azure and Heroku!
A number of people using recent sources experimenting with
caches will need to bust the cache at least once.  While we have
correct caching implemented now, the previous caching
implementation requires one busting in order to clear things
through.
moved to watch the community branch.
@bewest
Copy link
Member

bewest commented Jun 30, 2014

Looking good. I've tested this with @jwedding.

@bewest
Copy link
Member

bewest commented Jun 30, 2014

Many users will be required to do something like /?a=b or some similar shenanigans in order to bust their cache.

@bewest
Copy link
Member

bewest commented Jun 30, 2014

I saw the comments in some other places, feel free to link here.
Here's what I'm looking at:

I've tested it by hand, and I like the behavior. The important bit is that the audio resources do not re-request themselves once they've been loaded. The caching implemented here allows changing sources, refreshing, and getting fresh sources. It should also allow other caches to cache things.

More testers/analysis welcome.

@@ -20,7 +20,9 @@
var patientData = [];
var now = new Date().getTime();
var fs = require('fs');
var c = require("appcache-node");
var dateformat = require('dateformat');
Copy link
Contributor

Choose a reason for hiding this comment

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

dateformat does not seemed to be used???

nightscout.appcache is now a static file in /static
@brianhanifin
Copy link
Contributor Author

OK, I removed the unused packages and even moved the HTML 5 app cache file (nightscout.appcache) to a static file under /static, and removed the related code.

bewest and others added 3 commits June 30, 2014 18:19
The first bgdelta was always 0, which is incorrect.
This allows the first item to have a delta.
@brianhanifin
Copy link
Contributor Author

I also forget to mention that I changed the Travis and Dependencies links look to the /nightscout/ community branch

@bewest
Copy link
Member

bewest commented Jul 1, 2014

I saw that, thank you!

On Mon, Jun 30, 2014 at 8:21 PM, Brian Hanifin notifications@github.com
wrote:

I also forget to mention that I changed the Travis and Dependencies links
look to the /nightscout/ community branch


Reply to this email directly or view it on GitHub
#26 (comment)
.


$('#testAlarms').click(function(event) {
d3.select('.audio.alarms audio').each(function (data, i) {
var audio = this;
audio.load();
Copy link
Member

Choose a reason for hiding this comment

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

This load should also be deleted. Just play seems to work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking for confirmation I found this statement in HTML5 Audio - The State of Play:

Until fairly recently, the load method was required in order to tell the browser about a change in the media source and to get it to update appropriately. Now, media.load() causes the element to reset and to start selecting and loading a new media resource from scratch.

So I agree that this is probably unnecessary. Are you asking me to remark it out or delete it?

Copy link
Member

Choose a reason for hiding this comment

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

If you wouldn't mind deleting it, I can send you patch if you want.

I'm pretty happy with this, it's soft caching which does not prevent
updates.
While more caching could be done, I think simply removing all the loads
alone will accomplish quite a bit. Having added the soft caching like we
have will also continue to help.

This particular caching should not require any cache busting (except to fix
the overly aggressive cache now) once everyone is using this, we can
iterate on smarter or stronger or softer caching.

If Adrien agrees, let's roll with this soon.

-bewest

On Tue, Jul 1, 2014 at 12:40 PM, Brian Hanifin notifications@github.com
wrote:

In static/js/client.js:

 $('#testAlarms').click(function(event) {
  •    d3.select('.audio.alarms audio').each(function (data, i) {
    
  •      var audio = this;
    
  •      audio.load();
    

Looking for confirmation I found this statement in HTML5 Audio - The
State of Play http://html5doctor.com/html5-audio-the-state-of-play/:

Until fairly recently, the load method was required in order to tell the
browser about a change in the media source and to get it to update
appropriately. Now, media.load() causes the element to reset and to
start selecting and loading a new media resource from scratch.

So I agree that this is probably unnecessary. Are you asking me to
remark it out or delete it?


Reply to this email directly or view it on GitHub
https://github.com/nightscout/cgm-remote-monitor/pull/26/files#r14425040
.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

On July 1, 2014 at 12:54:16 PM, Ben West (notifications@github.com) wrote:

In static/js/client.js:

 $('#testAlarms').click(function(event) {
  •    d3.select('.audio.alarms audio').each(function (data, i) {
    
  •      var audio = this;
    
  •      audio.load();
    
    If you wouldn't mind deleting it, I can send you patch if you want. I'm pretty happy with this, it's soft caching which does not prevent updates. While more caching could be done, I think simply removing all the loads alone will accomplish quite a bit. Having added the soft caching like we have will also continue to help. This particular caching should not require any cache busting (except to fix the overly aggressive cache now) once everyone is using this, we can iterate on smarter or stronger or softer caching. If Adrien agrees, let's roll with this soon.


    Reply to this email directly or view it on GitHub.

bewest added a commit that referenced this pull request Jul 1, 2014
Express migration + website caching - merge fantastic work from @brianhanifin
@bewest bewest merged commit 233f1be into nightscout:master Jul 1, 2014
@brianhanifin brianhanifin deleted the express-cache branch July 1, 2014 23:40
brianhanifin added a commit that referenced this pull request Jul 23, 2014
Settings UI to switch to mmol display!
sulkaharo pushed a commit that referenced this pull request Aug 28, 2019
freddyyi pushed a commit to freddyyi/cgm-remote-monitor that referenced this pull request Apr 22, 2022
justinthomas pushed a commit to justinthomas/cgm-remote-monitor that referenced this pull request Nov 29, 2022
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

Successfully merging this pull request may close these issues.

4 participants