Serve Ember bootstrap HTML for all non-api requests #1788
Conversation
The backend no longer checks for an "html" in the `Accept` header. With the exception of 3 session related routes, all paths not starting with "/api" will be redirected to the static Ember bootstrap page. As a result of this change all non-api requests that don't contain "html" in the `Accept` header will now unconditionally return `200`, rather than `404`. In a sense, this expands the scope of rust-lang#556 to all requests, not just those that set the header. It also inverts the problem described in rust-lang#788, effectively turning it into a duplicate of rust-lang#556. Fixes: rust-lang#163
|
r? @sgrif (rust_highfive has picked a reviewer for you, use r? to override) |
| } else { | ||
| let is_backend_path = match req.path() { | ||
| // Special case routes used for authentication | ||
| "/authorize" | "/authorize_url" | "/logout" => true, |
There was a problem hiding this comment.
Having to special-case these is a bit unfortunate. Would it be possible to instead do something along these lines:
- Try to serve the request with the backend.
- On success, return the result.
- If the backend returns a 404, check whether the path starts with
/api.- If so, return a 404
- Otherwise redirect to
index.html.
Basically give the backend a chance to serve any request first, so we don't have to special-case any routes outside of the /api prefix. I haven't looked into the details, so I'm not sure whether this is feasible in this particular case, but it's roughly what I would do in other web frameworks.
There was a problem hiding this comment.
Is there a reason we can't just move these routes under the /api namespace? There's no reason we need the endpoint hit by the front end to be the same URL that the browser is currently on.
There was a problem hiding this comment.
If we move all backend routes into /api, we can also make nginx decide where to send requests:
/assets/→ serve from file system/api/→ proxy to backend- everything else → serve
index.html
There isn't really a reason why requests for index.html should go to the backend, since the file is actually part of the frontend.
There was a problem hiding this comment.
Is there a reason we can't just move these routes under the
/apinamespace?
I'd be fine with moving these. The reason I didn't move them at this point is that they are involved in session management, and at least one endpoint needs to be changed in parallel with changes to GitHub oauth settings so that the oauth callback from GitHub hit the right endpoint.
It would probably be best if we add duplicate routes under /api, deploy, change GitHub oauth settings, and then land a commit removing the old routes.
There was a problem hiding this comment.
+1 for moving them under /api. That also makes enabling FastBoot on the routes easier.
| } else { | ||
| let is_backend_path = match req.path() { | ||
| // Special case routes used for authentication | ||
| "/authorize" | "/authorize_url" | "/logout" => true, |
There was a problem hiding this comment.
Is there a reason we can't just move these routes under the /api namespace? There's no reason we need the endpoint hit by the front end to be the same URL that the browser is currently on.
|
☔ The latest upstream changes (presumably #1918) made this pull request unmergeable. Please resolve the merge conflicts. |
|
I'm closing this PR for now, as making this change will affect how crawlers see the site and a change to this logic may not make sense until we enable fastboot. |
The backend no longer checks for an "html" in the
Acceptheader.With the exception of 3 session related routes, all paths not starting
with "/api" will be redirected to the static Ember bootstrap page.
As a result of this change all non-api requests that don't contain
"html" in the
Acceptheader will now unconditionally return200,rather than
404. In a sense, this expands the scope of #556 to allrequests, not just those that set the header. It also inverts the
problem described in #788, effectively turning it into a duplicate
of #556.
Fixes: #163