Make / work under FastBoot #1937

kzys · 2019-12-05T14:52:56Z

This PR adds a transparent cache called "fetcher" to prevent making 2 XHRs
(both from the frontend to the backend).

rust-highfive · 2019-12-05T14:53:07Z

(rust_highfive has picked a reviewer for you, use r? to override)

This commit adds a transparent cache called "fetcher" to prevent making 2 XHRs (both from the frontend to the backend).

https://api.emberjs.com/ember/3.14/classes/Route/methods/setupController?anchor=setupController > If you implement the setupController hook in your Route, it will prevent this default behavior.

smarnach · 2019-12-16T10:55:29Z

@carols10cents @jtgeibel Would any of you be able to take this review? Frontend reviews tend to take me ages, since I just don't know JS and Ember very well, and currently I don't manage to find the time. (I hope this will get better again next year.)

carols10cents · 2019-12-16T14:46:19Z

No problem!

r? @carols10cents

carols10cents

Thank you so much for continuing to work on enabling fastboot! When I run the frontend locally with USE_FASTBOOT=1 npm run start:local, the homepage is loading with crate info whether I visit it with JavaScript on or off. Very neat!!!

I have a few questions about expected behavior:

When I have JavaScript disabled, I see the frontpage content, but I also see the "This site requires JavaScript to be enabled." still at the bottom of the page. Is this intentional? Screenshot:

When I have JavaScript disabled and try to log in, I see a full-page error that says window.open is not a function. I think we need JavaScript in order to log in? What are the plans for converting this? Could we show a "Logging in requires JavaScript" error message instead? This is also currently a problem with master when fastboot is enabled, but because the rest of the page doesn't fill in with data, it's less noticeable ;) When fastboot is off, I only get the "This site requires JavaScript to be enabled." message, without a login link.
This happens on master too, so perhaps this PR isn't meant to fix this issue, let me know. When I log in, it works, but as the popup window is closing I see this error saying window.close is not a function:

The popup window does close though, and I'm successfully logged in as I expect. It's just strange to see an error message briefly.

When this happens, in the frontend logs in the terminal, I see:

GET /api/private/session/authorize?code=[redacted]&state=[redacted] 200 3.041 ms - 49
Error while processing route: github-authorize window.close is not a function TypeError: window.close is not a function
    at Class._callee$ (/var/folders/qc/0ry2bjtd7wd9q8r1c7wylplr0000gn/T/broccoli-44493o2G1SkH1hxhM/out-418-broccoli_merge_trees/assets/cargo/routes/github-authorize.js:45:1)
    at tryCatch (/var/folders/qc/0ry2bjtd7wd9q8r1c7wylplr0000gn/T/broccoli-44493o2G1SkH1hxhM/out-418-broccoli_merge_trees/assets/vendor/regenerator-runtime/runtime.js:63:1)
    at Generator.invoke [as _invoke] (/var/folders/qc/0ry2bjtd7wd9q8r1c7wylplr0000gn/T/broccoli-44493o2G1SkH1hxhM/out-418-broccoli_merge_trees/assets/vendor/regenerator-runtime/runtime.js:337:1)
    at Generator.prototype.<computed> [as next] (/var/folders/qc/0ry2bjtd7wd9q8r1c7wylplr0000gn/T/broccoli-44493o2G1SkH1hxhM/out-418-broccoli_merge_trees/assets/vendor/regenerator-runtime/runtime.js:96:1)
    at tryCatch (/var/folders/qc/0ry2bjtd7wd9q8r1c7wylplr0000gn/T/broccoli-44493o2G1SkH1hxhM/out-418-broccoli_merge_trees/assets/vendor/regenerator-runtime/runtime.js:63:1)
    at invoke (/var/folders/qc/0ry2bjtd7wd9q8r1c7wylplr0000gn/T/broccoli-44493o2G1SkH1hxhM/out-418-broccoli_merge_trees/assets/vendor/regenerator-runtime/runtime.js:139:1)
    at /var/folders/qc/0ry2bjtd7wd9q8r1c7wylplr0000gn/T/broccoli-44493o2G1SkH1hxhM/out-418-broccoli_merge_trees/assets/vendor/regenerator-runtime/runtime.js:147:1
    at tryCatcher (/var/folders/qc/0ry2bjtd7wd9q8r1c7wylplr0000gn/T/broccoli-44493o2G1SkH1hxhM/out-418-broccoli_merge_trees/assets/rsvp.js:200:1)
    at invokeCallback (/var/folders/qc/0ry2bjtd7wd9q8r1c7wylplr0000gn/T/broccoli-44493o2G1SkH1hxhM/out-418-broccoli_merge_trees/assets/rsvp.js:372:1)
    at publish (/var/folders/qc/0ry2bjtd7wd9q8r1c7wylplr0000gn/T/broccoli-44493o2G1SkH1hxhM/out-418-broccoli_merge_trees/assets/rsvp.js:358:1)
    at /var/folders/qc/0ry2bjtd7wd9q8r1c7wylplr0000gn/T/broccoli-44493o2G1SkH1hxhM/out-418-broccoli_merge_trees/assets/ember-testing/ext/rsvp.js:14:1
    at invokeWithOnError (/var/folders/qc/0ry2bjtd7wd9q8r1c7wylplr0000gn/T/broccoli-44493o2G1SkH1hxhM/out-418-broccoli_merge_trees/assets/backburner.js:214:1)
    at Queue.flush (/var/folders/qc/0ry2bjtd7wd9q8r1c7wylplr0000gn/T/broccoli-44493o2G1SkH1hxhM/out-418-broccoli_merge_trees/assets/backburner.js:125:1)
    at DeferredActionQueues.flush (/var/folders/qc/0ry2bjtd7wd9q8r1c7wylplr0000gn/T/broccoli-44493o2G1SkH1hxhM/out-418-broccoli_merge_trees/assets/backburner.js:278:1)
    at Backburner.end (/var/folders/qc/0ry2bjtd7wd9q8r1c7wylplr0000gn/T/broccoli-44493o2G1SkH1hxhM/out-418-broccoli_merge_trees/assets/backburner.js:410:1)
    at Timeout.Backburner._boundAutorunEnd [as _onTimeout] (/var/folders/qc/0ry2bjtd7wd9q8r1c7wylplr0000gn/T/broccoli-44493o2G1SkH1hxhM/out-418-broccoli_merge_trees/assets/backburner.js:372:1)
    at listOnTimeout (internal/timers.js:531:17)
    at processTimers (internal/timers.js:475:7)
2019-12-16T15:22:21.733Z 200 OK /authorize/github?code=[redacted]&state=[redacted]

The nginx file is still only using fastboot for /policies, so I haven't tested this on staging yet.

kzys · 2019-12-16T17:28:02Z

Yes. Login, logout and "This site requires JavaScript to be enabled." message are known issues and tracked by #1811. I'd like to keep them open and address them separately, instead of having a big PR. Let me know if you (or the team) are not comfortable regrading the approach.

Regarding nginx.conf, can you review #1907? I'd like to have him changes on master first to avoid conflicts.

carols10cents · 2019-12-19T21:21:32Z

Yes. Login, logout and "This site requires JavaScript to be enabled." message are known issues and tracked by #1811. I'd like to keep them open and address them separately, instead of having a big PR. Let me know if you (or the team) are not comfortable regrading the approach.

Oooops. You caught me-- I haven't actually read through #1811 😆🤭 What you're doing is fine ❤️

Regarding nginx.conf, can you review #1907? I'd like to have him changes on master first to avoid conflicts.

#1907 has been merged in so I merged master into this branch and I'm now deploying this branch to staging to try the different modes out :)

carols10cents · 2019-12-20T16:15:58Z

Hmmm, so I have 9b4aaf5 deployed to staging, which contains the nginx changes, and USE_FASTBOOT is set to staging-experimental.

However, when I visit https://staging.crates.io/ with JavaScript disabled, I'm seeing only the "This site requires JavaScript to be enabled." notice. I'm not sure why...

kzys · 2019-12-20T16:39:55Z

Yeah, that's odd and I can repro the issue. Seems CloudFront is not caching and the response is coming from Express. Then it should be server-rendered, but it's not.

% curl --dump-header - --silent -H 'Accept: text/html' 'https://staging.crates.io/'                                                                                       
HTTP/2 200                                                                                                                                                                
content-type: text/html; charset=UTF-8
content-length: 2952
server: nginx
date: Fri, 20 Dec 2019 16:38:30 GMT
x-powered-by: Express
accept-ranges: bytes
cache-control: public, max-age=0
last-modified: Thu, 19 Dec 2019 21:30:20 GMT
etag: W/"b88-16f201191e0"
strict-transport-security: max-age=31536000
via: 1.1 vegur, 1.1 9046e5a276a05e60ee34c8475e92b8e7.cloudfront.net (CloudFront)
vary: Accept-Encoding,Accept,Accept-Encoding,Cookie
x-cache: Miss from cloudfront
x-amz-cf-pop: SEA19-C2
x-amz-cf-id: 4R1Ze01jN1Ytyp4Nwdr6JaMVXzHVVp2bL3LlpGJqaVL5YzYDj8vwnA==
...

carols10cents · 2019-12-20T18:34:29Z

Yeah I have no idea either.

I'm going to deploy master to staging now, in prep for doing a deploy to prod. I'll put staging back to this branch with the env var set when I'm done!

kzys · 2019-12-20T23:36:45Z

Okay. Can you add LANG=en_US.UTF-8 on Config Vars after the staging deployment (or do you have something else on the staging)?

Ruby's strings have encodings and POSIX's locale-related environment variables affect the default encoding. It worked for me at least.

carols10cents · 2019-12-23T20:53:43Z

Ok, I have 9b4aaf5 deployed to staging right now, and I've set the LANG environment variable to en_US.UTF-8 and USE_FASTBOOT to staging-experimental. However, I'm still seeing the same result-- when I visit https://staging.crates.io/, I only see the "This site requires JavaScript to be enabled." message.

$ curl --dump-header - --silent -H 'Accept: text/html' 'https://staging.crates.io/'
HTTP/2 200
content-type: text/html; charset=UTF-8
content-length: 2952
server: nginx
date: Mon, 23 Dec 2019 20:52:09 GMT
x-powered-by: Express
accept-ranges: bytes
cache-control: public, max-age=0
last-modified: Mon, 23 Dec 2019 20:01:53 GMT
etag: W/"b88-16f345a0768"
strict-transport-security: max-age=31536000
via: 1.1 vegur, 1.1 2f66aa06710fece8ed203ab0ea81eb56.cloudfront.net (CloudFront)
vary: Accept-Encoding,Accept,Accept-Encoding,Cookie
x-cache: Miss from cloudfront
x-amz-cf-pop: IAD89-C3
x-amz-cf-id: qkcJuIOGIIf1XEUBtdpItKlIprH_RGkx3DasBqKfJXLb9B1yUyphcg==

kzys · 2019-12-24T01:56:13Z

Here is mine: https://fathomless-island-39956.herokuapp.com/

I think @jtgeibel might have a working environment as well.

carols10cents · 2019-12-31T01:32:23Z

@jtgeibel when you get a chance, since you have access to staging now, could you compare your setup and staging's and see if I missed any environment variables or anything?

jtgeibel · 2020-01-03T03:11:26Z

@carols10cents I've pushed this PR to my environment, and added you to the app on Heroku. Are things working for you at https://jtgeibel-staging-crates-io.herokuapp.com/ ? I've done some fastboot testing in this environment before, but don't think I've tried this particular PR yet. It seems to be working fine for me.

I'm pushing this back to staging now as well, to see if I can replicate the issue you were seeing there.

jtgeibel · 2020-01-03T03:48:04Z

I've added Papertrail to staging and have been looking at the logs. It looks like in the staging environment, the request is coming with a path of /index.html instead of /, even when using curl. Is CloudFront or something else possibly rewriting this before it gets to the app?

kzys · 2020-01-03T04:12:42Z

@jtgeibel Interesting. Seems like it is.

@pietroalbini Do we really need the default root object?

There is a concern that setting the default root object to index.html for the crates.io webapp causes Fastboot to break: rust-lang/crates.io#1937 (comment) This adds a temporary feature flag to remove the default root object just from staging, without affecting production. After testing is done the feature flag will have to be removed, either disabling or enabling the root object both on staging and production.

pietroalbini · 2020-01-03T09:00:23Z

@Ksys I don't think so for the webapp. Removed the root object on staging (without touching prod), the change should propagate in around 15 minutes.

carols10cents · 2020-01-03T20:03:26Z

It's working!!! Thank you all for helping to figure this out!!

I also tested unsetting USE_FASTBOOT and everything works fine, so I think the cloudfront root object change is safe to make on prod, and this is safe to merge.

@bors r+

bors · 2020-01-03T20:03:27Z

📌 Commit 9b4aaf5 has been approved by carols10cents

Make / work under FastBoot This PR adds a transparent cache called "fetcher" to prevent making 2 XHRs (both from the frontend to the backend).

bors · 2020-01-03T20:03:35Z

⌛ Testing commit 9b4aaf5 with merge a34f6c4...

bors · 2020-01-03T20:09:28Z

☀️ Test successful - checks-travis
Approved by: carols10cents
Pushing a34f6c4 to master...

@locks

index: Replace loading spinner with list item placeholders #1937 changed the data loading code of the `index` route to load the summary data in the `model()` hook again. That is fine when using fastboot, but without fastboot it is currently resulting in an ugly loading spinner page before the real page content gets rendered. This PR replaces the loading page with rendering the real page directly and showing placeholders until the real data is available. The new approach is still compatible with fastboot due to the use of `deferReadiness()`. ![loading](https://user-images.githubusercontent.com/141300/80317190-98050780-8802-11ea-8596-8d9f9ee05545.gif) r? @locks

rust-highfive assigned smarnach Dec 5, 2019

rust-highfive added the S-waiting-on-review label Dec 5, 2019

kzys force-pushed the fastboot-index branch 2 times, most recently from fb0fca0 to 024040c Compare December 5, 2019 15:05

Make / work under FastBoot

eeeacb7

This commit adds a transparent cache called "fetcher" to prevent making 2 XHRs (both from the frontend to the backend).

kzys force-pushed the fastboot-index branch from 024040c to eeeacb7 Compare December 5, 2019 15:18

Fix non-FastBoot case

380b07d

https://api.emberjs.com/ember/3.14/classes/Route/methods/setupController?anchor=setupController > If you implement the setupController hook in your Route, it will prevent this default behavior.

kzys force-pushed the fastboot-index branch from e0f60a4 to 380b07d Compare December 10, 2019 06:33

kzys mentioned this pull request Dec 10, 2019

Tracking issue for Ember FastBoot #1811

Closed

19 tasks

rust-highfive assigned carols10cents and unassigned smarnach Dec 16, 2019

carols10cents requested changes Dec 16, 2019

View reviewed changes

carols10cents added S-waiting-on-author and removed S-waiting-on-review labels Dec 16, 2019

Merge remote-tracking branch 'upstream/master' into fastboot-index

9b4aaf5

bors added a commit that referenced this pull request Jan 3, 2020

Auto merge of #1937 - kzys:fastboot-index, r=carols10cents

a34f6c4

Make / work under FastBoot This PR adds a transparent cache called "fetcher" to prevent making 2 XHRs (both from the frontend to the backend).

carols10cents approved these changes Jan 3, 2020

View reviewed changes

bors merged commit 9b4aaf5 into rust-lang:master Jan 3, 2020

kzys deleted the fastboot-index branch January 3, 2020 21:42

Turbo87 mentioned this pull request Apr 26, 2020

index: Replace loading spinner with list item placeholders #2457

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make / work under FastBoot #1937

Make / work under FastBoot #1937

kzys commented Dec 5, 2019

rust-highfive commented Dec 5, 2019

smarnach commented Dec 16, 2019

carols10cents commented Dec 16, 2019

carols10cents left a comment •

edited

Loading

kzys commented Dec 16, 2019 •

edited

Loading

carols10cents commented Dec 19, 2019

carols10cents commented Dec 20, 2019

kzys commented Dec 20, 2019 •

edited

Loading

carols10cents commented Dec 20, 2019

kzys commented Dec 20, 2019

carols10cents commented Dec 23, 2019

kzys commented Dec 24, 2019

carols10cents commented Dec 31, 2019

jtgeibel commented Jan 3, 2020

jtgeibel commented Jan 3, 2020

kzys commented Jan 3, 2020 •

edited

Loading

pietroalbini commented Jan 3, 2020

carols10cents commented Jan 3, 2020

bors commented Jan 3, 2020

bors commented Jan 3, 2020

bors commented Jan 3, 2020

Make / work under FastBoot #1937

Make / work under FastBoot #1937

Conversation

kzys commented Dec 5, 2019

rust-highfive commented Dec 5, 2019

smarnach commented Dec 16, 2019

carols10cents commented Dec 16, 2019

carols10cents left a comment • edited Loading

Choose a reason for hiding this comment

kzys commented Dec 16, 2019 • edited Loading

carols10cents commented Dec 19, 2019

carols10cents commented Dec 20, 2019

kzys commented Dec 20, 2019 • edited Loading

carols10cents commented Dec 20, 2019

kzys commented Dec 20, 2019

carols10cents commented Dec 23, 2019

kzys commented Dec 24, 2019

carols10cents commented Dec 31, 2019

jtgeibel commented Jan 3, 2020

jtgeibel commented Jan 3, 2020

kzys commented Jan 3, 2020 • edited Loading

pietroalbini commented Jan 3, 2020

carols10cents commented Jan 3, 2020

bors commented Jan 3, 2020

bors commented Jan 3, 2020

bors commented Jan 3, 2020

carols10cents left a comment •

edited

Loading

kzys commented Dec 16, 2019 •

edited

Loading

kzys commented Dec 20, 2019 •

edited

Loading

kzys commented Jan 3, 2020 •

edited

Loading