Skip to content

Serve Ember bootstrap HTML for all non-api requests #1788

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Serve Ember bootstrap HTML for all non-api requests #1788

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 12 additions & 13 deletions src/middleware/ember_index_rewrite.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,21 +25,20 @@ impl AroundMiddleware for EmberIndexRewrite {

impl Handler for EmberIndexRewrite {
fn call(&self, req: &mut dyn Request) -> Result<Response, Box<dyn Error + Send>> {
// If the client is requesting html, then we've only got one page so
// rewrite the request.
let wants_html = req
.headers()
.find("Accept")
.map(|accept| accept.iter().any(|s| s.contains("html")))
.unwrap_or(false);
// If the route starts with /api, just assume they want the API
// response and fall through.
let is_api_path = req.path().starts_with("/api");
let handler = self.handler.as_ref().unwrap();
if wants_html && !is_api_path {
handler.call(&mut RequestProxy::rewrite_path(req, "/index.html"))
} else {
let is_backend_path = match req.path() {
// Special case routes used for authentication
"/authorize" | "/authorize_url" | "/logout" => true,
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is there a reason we can't just move these routes under the /api namespace?

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 for moving them under /api. That also makes enabling FastBoot on the routes easier.

// Paths starting with `/api` are intended for the backend
path if path.starts_with("/api") => true,
_ => false,
};

if is_backend_path {
handler.call(req)
} else {
// Serve static Ember page to bootstrap the frontend
handler.call(&mut RequestProxy::rewrite_path(req, "/index.html"))
}
}
}
2 changes: 2 additions & 0 deletions src/router.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,8 @@ pub fn build_router(app: &App) -> R404 {
router.head("/api/v1/*path", R(Arc::clone(&api_router)));
router.delete("/api/v1/*path", R(api_router));

// These routes are special cased in the EmberIndexRewrite middleware.
// Avoid adding new ones and keep in sync with the list there!
router.get("/authorize_url", C(user::session::github_authorize));
router.get("/authorize", C(user::session::github_access_token));
router.delete("/logout", C(user::session::logout));
Expand Down