Remove Hypothesis client from service

[robertknight]

Do not merge until Monday 2017-03-06 Go for it!

This PR removes the ability for the service to serve the Hypothesis client directly in favor of always loading the client from an external URL, defaulting to cdn.hypothes.is

The changes are:

1. Remove code paths in client views (/app.html, /embed.js) for serving client assets directly, keeping just the versions which use the externally hosted boot script
2. Remove the client asset environment which supported the code paths in (1)
3. Remove the 'hypothesis' npm package which provided the assets used by (2)

[codecov-io]

Codecov Report

Merging #4399 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #4399      +/-   ##
==========================================
+ Coverage   94.26%   94.26%   +<.01%     
==========================================
  Files         320      320              
  Lines       18173    18124      -49     
  Branches     1074     1071       -3     
==========================================
- Hits        17130    17084      -46     
+ Misses        935      929       -6     
- Partials      108      111       +3

Impacted Files	Coverage Δ
tests/h/views/main_test.py	97.89% <ø> (-0.09%)	❌
h/assets.py	86.27% <ø> (+4.13%)	✅
tests/h/routes_test.py	100% <ø> (ø)	✅
h/routes.py	100% <ø> (ø)	✅
h/models/feature.py	93.1% <ø> (ø)	✅
h/views/client.py	77.27% <100%> (-5.78%)	❌
tests/h/views/client_test.py	100% <100%> (ø)	✅
h/stats.py	83.33% <0%> (ø)	✅
src/memex/search/init.py	38.88% <0%> (ø)	✅
h/session.py	91.83% <0%> (ø)	✅

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 78e9b08...c6a0c34. Read the comment docs.

[codecov-io]

Codecov Report

Merging #4399 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #4399      +/-   ##
==========================================
+ Coverage   94.26%   94.26%   +<.01%     
==========================================
  Files         320      320              
  Lines       18173    18124      -49     
  Branches     1074     1071       -3     
==========================================
- Hits        17130    17084      -46     
+ Misses        935      929       -6     
- Partials      108      111       +3

Impacted Files	Coverage Δ
tests/h/views/main_test.py	97.89% <ø> (-0.09%)	❌
h/assets.py	86.27% <ø> (+4.13%)	✅
tests/h/routes_test.py	100% <ø> (ø)	✅
h/routes.py	100% <ø> (ø)	✅
h/models/feature.py	93.1% <ø> (ø)	✅
h/views/client.py	77.27% <100%> (-5.78%)	❌
tests/h/views/client_test.py	100% <100%> (ø)	✅
h/stats.py	83.33% <0%> (ø)	✅
src/memex/search/init.py	38.88% <0%> (ø)	✅
h/session.py	91.83% <0%> (ø)	✅

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 78e9b08...c6a0c34. Read the comment docs.

[nickstenning]

There's also stuff in the Dockerfile which can be simplified now.

[nickstenning]

There's also stuff in the Dockerfile which can be simplified now.

[robertknight]

There's also stuff in the Dockerfile which can be simplified now.

Just this line or is there anything else?

[robertknight]

There's also stuff in the Dockerfile which can be simplified now.

Just this line or is there anything else?

[seanh]

There's still some mentions of <script src="https://hypothes.is/embed.js" async></script> in the docs directory. Looks like this 302s to cdn.hypothes.is/hypothesis now. Do we want to leave it like that or we we want to change the docs to say <script src="cdn.hypothes.is/hypothesis" directly?

[seanh]

There's still some mentions of <script src="https://hypothes.is/embed.js" async></script> in the docs directory. Looks like this 302s to cdn.hypothes.is/hypothesis now. Do we want to leave it like that or we we want to change the docs to say <script src="cdn.hypothes.is/hypothesis" directly?

[robertknight]

I think we can leave it like that for now and reconsider whether to change the publicly advertised entry point in future. cdn.hypothes.is is an "implementation detail" for most users at the moment, unless they have strict CSP policies.

[robertknight]

I think we can leave it like that for now and reconsider whether to change the publicly advertised entry point in future. cdn.hypothes.is is an "implementation detail" for most users at the moment, unless they have strict CSP policies.

[seanh]

So it's deliberate that we ourselves even continue to use the /embed.js URL that 302s to the CDN, instead of just using the CDN URL directly? For example in /app.html it's still <script src="/embed.js"></script> and not <script src="https://cdn.hypothes.is/hypothesis"></script>

I understand that we might want to keep the /embed.js URL around and have it 302, and maybe even leave our public docs still saying /embed.js, but it seems that, given that h.client_url is configurable, things like app.html could just use client_url directly instead of going via /embed.js?

(I see that /embed.js is cached though)

[seanh]

So it's deliberate that we ourselves even continue to use the /embed.js URL that 302s to the CDN, instead of just using the CDN URL directly? For example in /app.html it's still <script src="/embed.js"></script> and not <script src="https://cdn.hypothes.is/hypothesis"></script>

I understand that we might want to keep the /embed.js URL around and have it 302, and maybe even leave our public docs still saying /embed.js, but it seems that, given that h.client_url is configurable, things like app.html could just use client_url directly instead of going via /embed.js?

(I see that /embed.js is cached though)

[seanh]

Should use_client_boot_script be added to FEATURES_PENDING_REMOVAL?

[seanh]

Should use_client_boot_script be added to FEATURES_PENDING_REMOVAL?

[seanh]

Can the assets_client route be removed from routes.py?

[seanh]

Can the assets_client route be removed from routes.py?

[seanh]

I think I share the concern that you mentioned in Slack about potential confusion with dev envs. It looks like by default https://localhost:5000/docs/help will make a request to localhost/embed.js which will 302 to https://cdn.hypothes.is/hypothesis. For some reason I'm also seeing a second embed.js request, this time to hypothes.is/embed.js, as well. And then the sidebar gets loaded from cdn.hypothes.is, I login to it with my production hypothesis account and it sends annotations to the production hypothesis API, even though I'm on a page of my development web service that happens to have an embedded client on the page. Definitely seems a bit confusing.

We could just set h.client_url to http://localhost:3001/hypothesis in development-app.ini? This of course would not work if you didn't have a dev client installed and running gulp watch. But it seems less bad than embedding the prod client in a dev web service. You just get a broken /docs/help page (and any other page that embeds), and in the network logs you can see why, rather than a working client that you won't notice is a prod client.

[seanh]

I think I share the concern that you mentioned in Slack about potential confusion with dev envs. It looks like by default https://localhost:5000/docs/help will make a request to localhost/embed.js which will 302 to https://cdn.hypothes.is/hypothesis. For some reason I'm also seeing a second embed.js request, this time to hypothes.is/embed.js, as well. And then the sidebar gets loaded from cdn.hypothes.is, I login to it with my production hypothesis account and it sends annotations to the production hypothesis API, even though I'm on a page of my development web service that happens to have an embedded client on the page. Definitely seems a bit confusing.

We could just set h.client_url to http://localhost:3001/hypothesis in development-app.ini? This of course would not work if you didn't have a dev client installed and running gulp watch. But it seems less bad than embedding the prod client in a dev web service. You just get a broken /docs/help page (and any other page that embeds), and in the network logs you can see why, rather than a working client that you won't notice is a prod client.

[seanh]

Hmm, I think I'm gonna have to figure out how to run the client repo's gulp watch over HTTPS :). I still can't run the local dev web service over HTTP (it still looks up Chrome horrifically due to gunicorn running out of workers), and when running h over HTTPS pages like /docs/help can't embed my local dev client unless that is also HTTPS. (For now I can run h over HTTP and just increase the number of gunicorn workers so it doesn't lock up.)

[seanh]

Hmm, I think I'm gonna have to figure out how to run the client repo's gulp watch over HTTPS :). I still can't run the local dev web service over HTTP (it still looks up Chrome horrifically due to gunicorn running out of workers), and when running h over HTTPS pages like /docs/help can't embed my local dev client unless that is also HTTPS. (For now I can run h over HTTP and just increase the number of gunicorn workers so it doesn't lock up.)

[seanh]

Regarding the /embed.js URL vs the CDN URL - I wonder if it might be worth coming up with two distinct names for these two URLs and using them consistently?

For example in views/client.py there's DEFAULT_CLIENT_URL = 'https://cdn.hypothes.is/hypothesis' (and also the h.client_url setting and env var that can override this), so it seems like "the client URL" is the CDN URL. But then further down in the file there is client_url = request.route_path('embed'), another variable named client url but holding a different URL (a URL which, when loaded, 302s to the first client URL).

I think it might be a little confusing.

I think "client url" is a good name for the CDN URL and the DEFAULT_CLIENT_URL / h.client_url settings, and maybe just "embed url" should be used consistently as the name for the embed.js URL (and its associated route and view callable). The embed URL is the one that people embed into pages, the client URL is the one where the client is actually served.

[seanh]

Regarding the /embed.js URL vs the CDN URL - I wonder if it might be worth coming up with two distinct names for these two URLs and using them consistently?

For example in views/client.py there's DEFAULT_CLIENT_URL = 'https://cdn.hypothes.is/hypothesis' (and also the h.client_url setting and env var that can override this), so it seems like "the client URL" is the CDN URL. But then further down in the file there is client_url = request.route_path('embed'), another variable named client url but holding a different URL (a URL which, when loaded, 302s to the first client URL).

I think it might be a little confusing.

I think "client url" is a good name for the CDN URL and the DEFAULT_CLIENT_URL / h.client_url settings, and maybe just "embed url" should be used consistently as the name for the embed.js URL (and its associated route and view callable). The embed URL is the one that people embed into pages, the client URL is the one where the client is actually served.

[robertknight]

Should use_client_boot_script be added to FEATURES_PENDING_REMOVAL?

Done.

Can the assets_client route be removed from routes.py?

Done.

But it seems less bad than embedding the prod client in a dev web service

The plan for this is that the /docs/help page will by default load the production client but use the local service for storage. This means that you can develop the web service without needing to set up a build of the client. The way this will be done is by having pages such as /docs/help include a config script tag which causes the client to load app.html from the local service (which includes the API endpoints from the local service) but use the production client assets. Does this make sense to you?

I'd like to implement this as a separate change from this PR however.

[robertknight]

Should use_client_boot_script be added to FEATURES_PENDING_REMOVAL?

Done.

Can the assets_client route be removed from routes.py?

Done.

But it seems less bad than embedding the prod client in a dev web service

The plan for this is that the /docs/help page will by default load the production client but use the local service for storage. This means that you can develop the web service without needing to set up a build of the client. The way this will be done is by having pages such as /docs/help include a config script tag which causes the client to load app.html from the local service (which includes the API endpoints from the local service) but use the production client assets. Does this make sense to you?

I'd like to implement this as a separate change from this PR however.

[robertknight]

Hmm, I think I'm gonna have to figure out how to run the client repo's gulp watch over HTTPS

It is possible to make an express server use SSL. For the purposes of testing this out I suggest just configuring gunicorn to use a different type of worker locally.

[robertknight]

Hmm, I think I'm gonna have to figure out how to run the client repo's gulp watch over HTTPS

It is possible to make an express server use SSL. For the purposes of testing this out I suggest just configuring gunicorn to use a different type of worker locally.

[seanh]

The plan for this is that the /docs/help page will by default load the production client but use the
local service for storage. This means that you can develop the web service without needing to set up a
build of the client. The way this will be done is by having pages such as /docs/help include a config
script tag which causes the client to load app.html from the local service (which includes the API
endpoints from the local service) but use the production client assets. Does this make sense to you?

Yeah, that makes sense. So you want to merge it as is (dev site embeds prod client that talks to prod service) for now?

[seanh]

The plan for this is that the /docs/help page will by default load the production client but use the
local service for storage. This means that you can develop the web service without needing to set up a
build of the client. The way this will be done is by having pages such as /docs/help include a config
script tag which causes the client to load app.html from the local service (which includes the API
endpoints from the local service) but use the production client assets. Does this make sense to you?

Yeah, that makes sense. So you want to merge it as is (dev site embeds prod client that talks to prod service) for now?

[robertknight]

So you want to merge it as is (dev site embeds prod client that talks to prod service) for now?

Yes please, I'll follow up with a small PR to resolve the issue for /docs/help separately.

[robertknight]

So you want to merge it as is (dev site embeds prod client that talks to prod service) for now?

Yes please, I'll follow up with a small PR to resolve the issue for /docs/help separately.

[seanh]

Ok, this sounds good to me, just waiting for a response to my question about whether we ourselves want to be using embed.js or just using the CDN URL directly? And to my suggestion about naming the embed.js and the CDN URLs more distinctly in the code.

[seanh]

Ok, this sounds good to me, just waiting for a response to my question about whether we ourselves want to be using embed.js or just using the CDN URL directly? And to my suggestion about naming the embed.js and the CDN URLs more distinctly in the code.

[seanh]

I'm still seeing references to app_js in some tests, though I guess they don't do any harm

[seanh]

I'm still seeing references to app_js in some tests, though I guess they don't do any harm

[seanh]

Ok, looks good to me. Great code surgery @robertknight, loads of code and complexity removed here, and also releasing, deploying and setting up dev envs for everything is becoming steadily simpler and more decoupled

[seanh]

Ok, looks good to me. Great code surgery @robertknight, loads of code and complexity removed here, and also releasing, deploying and setting up dev envs for everything is becoming steadily simpler and more decoupled