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

FastBoot Bug - Location none doesn't respect rootURL #13464

Closed
jasonmit opened this issue May 8, 2016 · 6 comments · Fixed by #13520
Closed

FastBoot Bug - Location none doesn't respect rootURL #13464

jasonmit opened this issue May 8, 2016 · 6 comments · Fixed by #13520

Comments

@jasonmit
Copy link
Member

jasonmit commented May 8, 2016

Location none doesn't respect the router's rootURL. This causes issues while generating urls (link-to's for example will omit the rootURL). Compare this to history's formatURL and you'll see the difference between implementations.

This is causing issues within the non-browser path of the visit API since it always defaults to the none location.

I would be happy to help patch, but first would like to hear feedback though before venturing off. For example, do we want to stub out the window.location API when in Node and stick with using the history location, do we patch the none location to respect rootURL in formatURL, or something I'm not thinking of?

Part of this is related to:
ember-fastboot/fastboot#53

@jasonmit jasonmit changed the title Location none doesn't respect rootURL FastBoot Bug - Location none doesn't respect rootURL May 8, 2016
@pixelhandler
Copy link
Contributor

@jasonmit perhaps the first step is a failing test

@jasonmit
Copy link
Member Author

jasonmit commented May 9, 2016

Sure, I'll add some failing tests on the location none. I'm also interested in gather feedback to see what the preferred fix may be, this way I don't waste cycles iterating on something that might not be wanted.

Until then, the reproduction steps are:

  • set rootURL to /foo/
  • set location to none
  • add a simple route this.route('bar');
  • in application template {{link-to 'bar' 'bar'}}
  • run app, observe the href of the bar link, notice no /foo/ prefix
  • switch location to auto and repeat last step

@jasonmit
Copy link
Member Author

jasonmit commented May 9, 2016

Here are two failing tests on formatURL and getURL with relation to rootURL: jasonmit@ae4e7b2

It didn't appear there was test coverage around location none.

/cc @pixelhandler

@tomdale
Copy link
Member

tomdale commented May 16, 2016

I think we should definitely add support for rootURL to the none location; the point is that it's an API-compatible version of the other Locations, but without side-effects. This IMO is clearly breaking compatibility with other locations, so should be considered a bug.

@jasonmit
Copy link
Member Author

Thanks for getting back, I'll take a pass at fixing this either tonight or tomorrow.

@jasonmit
Copy link
Member Author

jasonmit commented May 18, 2016

@tomdale ready for you to review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants