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

FastBootLocation API #195

Merged
merged 1 commit into from
Jun 4, 2016
Merged

Conversation

arjansingh
Copy link
Contributor

@arjansingh arjansingh commented May 14, 2016

@rwjblue So this is a first draft. There are a couple of issues:

  1. replaceWith and intermediateTransitionTo die hard. Specifically, setURL is not getting the path to redirect to when those two functions are called. Check the failing tests in TravisCI to see those examples.
  2. transitionTo still renders the body. The middleware or fastboot could be modified to not include it, but a better solution would be to tell Ember to abort the page rendering. Tips on how to do that would be appreciated. I couldn't find anything straightforward in the docs.

If we can solve those, I can add a few more test cases (covering model, and afterModel hooks come to mind) and get this ready for submission.

@arjansingh arjansingh changed the title [DRAFT] FastbootLocation API [DRAFT] FastBootLocation API May 14, 2016
@arjansingh
Copy link
Contributor Author

arjansingh commented May 16, 2016

@arjansingh
Copy link
Contributor Author

Update:

  1. replaceWith works! Coder error. My test application had a type in the route I was testing. 😛
  2. intermediateTransitionTo still has issues. I'm looking for suggestions on how to handle it since it does not call setURL or any other Location API functions.
  3. body still rendering inside of transitionTo and replaceWith. Any ideas on how to abort rendering? Or should I just delete the result body before returning it to the middleware?

get
} = Ember;

const TEMPORARY_REDIRECT_CODE = 307;
Copy link
Contributor

@marcoow marcoow May 17, 2016

Choose a reason for hiding this comment

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

I think we should just use 302 here. 307 was added so that servers can require subsequent requests after redirect responses to use the same request method as the original request that resulted in the redirect. While we want the same request method to be used of course, 307 was really only added for everythong other than GET and FastBoot is only ever dealing with GET anyway (in fact all other request methods expect for HEAD maybe) should actually be ignored by the FastBoot middleware - not sure whether that's actually the case yet.

Copy link
Member

Choose a reason for hiding this comment

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

302 is FOUND, whereas 307 is TEMPORARY REDIRECT, which I'd 307 is more semantically correct. The following note is from the 302 spec (emphasis mine):

Note: RFC 1945 and RFC 2068 specify that the client is not allowed to change the method on the redirected request. However, most existing user agent implementations treat 302 as if it were a 303 response, performing a GET on the Location field-value regardless of the original request method. The status codes 303 and 307 have been added for servers that wish to make unambiguously clear which kind of reaction is expected of the client.

302 has ambiguity in its handling, whereas 307 is clear

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed on 307 - TEMPORARY REDIRECT. If we were trying to maintain HTTP 1.0 compatibility 302 - FOUND would make sense. As it is, 307 is a very clear spec. We could make the code configurable with a 307 default, so people can override it. Would anyone object to that?

Copy link
Contributor

Choose a reason for hiding this comment

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

Making the exact code configurable sounds fine. Didn't want to start an argument about HTTP semantics here - I think that could easily get out of control ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

WIll do.

@danmcclain
Copy link
Member

As intermediateTransitionTo is not a final state (most often used to transition to a loading state), we should probably leave it as is. The page should do another transition before the final rendering, I would think. @rwjblue @tomdale concur?

@arjansingh
Copy link
Contributor Author

arjansingh commented May 17, 2016

@danmcclain This issue I'm encountering with intermediateTransitionTo right now is that it is not even transitioning correctly to the final route.

For its functionality to be maintained, it needs to:

  1. Not change the requested path
  2. Insert the content from the intermediate transition into the body

Currently, 1 happens, but 2 does not. Any thoughts on handeling this? My thought is that if you get an intermediateTransitionTo we just return index.html and just "slowboot" everything on the client side like a normal Ember app.

Update: I looked into my implementation and cannot think of a way to do it without messing with Ember.Router because it preempts the router's finalize transition code.

It's definitely something we'll need to discuss at sync up.

@rwjblue
Copy link
Member

rwjblue commented May 27, 2016

@arjansingh and I dug into this a bit today, and have determined that the issue mentioned above is not a blocker for this PR. Basically, in Ember if you do an intermediate transition during another transition you must abort the initial transition. When discovering that we also uncovered a larger issue WRT aborted transitions rejection and that error being unhandled.

@tomdale - This is good to go from my perspective...
@arjansingh - Can you open an issue for the unhandled TransitionAborted errors?

@arjansingh arjansingh changed the title [DRAFT] FastBootLocation API FastBootLocation API May 27, 2016
@arjansingh
Copy link
Contributor Author

Alright updated tests to disable the intermediateTransitionTo test. We can modify and reenable it when we resolve #202.

I also will update assertions for the other location tests to check for appropriate body content when ember-fastboot/fastboot#64 is released with the next fastboot beta.

@arjansingh arjansingh force-pushed the redirect branch 3 times, most recently from ac1f9b3 to 85418b1 Compare June 2, 2016 21:45
response.headers.set('location', redirectURL);
}

response.headers.set('x-fastboot-path', path); // for testing and debugging
Copy link
Member

Choose a reason for hiding this comment

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

I would only display this header when you enable it via a config flag, otherwise we leak what is running

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 We can config this.

Supports http redirection in the cases of `Route.transitionTo` and `Route.replaceWith`
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.

None yet

4 participants