Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

fix($location): store REQUEST_URI in cookie #6303

Closed
wants to merge 1 commit into from
Closed

fix($location): store REQUEST_URI in cookie #6303

wants to merge 1 commit into from

Conversation

nolazybits
Copy link
Contributor

Request Type: feature

How to reproduce:

Component(s): $location

Impact:

Complexity: small

This issue is related to:

Detailed Description:

With IE8 and html5mode, reloading the page to add hashbang lose server variables

Other Comments:

To fix issue #6195, setting the REQUEST_URI in a cookie before refreshing the page (for IE8 in html5 mode), and remove the cookie after the refresh is done.

What has been done:

  • adding pathInCookie (bool) to add the REQUEST_URI in a cookie to not loose it after refresh (IE8 in html5 mode) and pathInCookieName (string) to specify the name of the cookie. Default will be "angular"

If anybody can help me to create testing case for this pull request that would be awesome

@mary-poppins
Copy link

Thanks for the PR! Please check the items below to help us merge this faster. See the contributing docs for more information.

  • Uses the issue template (#6303)

If you need to make changes to your pull request, you can update the commit with git commit --amend.
Then, update the pull request with git push -f.

Thanks again for your help!

…ide when using IE8 in html5mode

- add pathInCookie (bool) to add the REQUEST_URI in a cookie to not loose it after refresh (IE8 in html5 mode)
- add pathInCookieName to specify the name of the cookie. Default will be "angular"

Closes #6195
@nolazybits
Copy link
Contributor Author

Have updated the commits to follow guideline provided.

Still don't know how to create test case for this bug fix as we need to emulate IE8 in html5mode.
So no test cases have been created for this bug fix but the current existing ones don't break.

@IgorMinar IgorMinar self-assigned this Feb 18, 2014
@nolazybits
Copy link
Contributor Author

Have used the template generator.
I still need help for the test case if they are needed?

I would love feedback. Kinda need those changes in the main stream (hopefully) as my company is nervous about using custom code (deviate from the main stream, to a point we can't update anymore)

Cheers.

@IgorMinar
Copy link
Contributor

Hi there, this change is unlikely to make it into the the master because it's not aligned with the responsibilities of $location.

Hash part of the url is never transmitted to the server because that's how browsers work. The whole hashbang thing is a hack to get around this limitation. The proper solution is to use history push/pop state but those are not available in old browsers.

I'll comment on the original issue with a suggested solution

@IgorMinar IgorMinar closed this Feb 27, 2014
@nolazybits
Copy link
Contributor Author

"Hash part of the url is never transmitted to the server because that's how browsers work. The whole hashbang thing is a hack to get around this limitation. The proper solution is to use history push/pop state but those are not available in old browsers."

Well, let's take an example. I have a sessionless webapp. I need to pass some get parameters to the application.

http://www.myapp.com/?token=123456789

Works fine in new browser, but with IE8 in html5mode the page get refresh and the url becomes
(depending on the prefix you use, here only #)

http://www.myapp.com/#/?token=123456789

This breaks my application in IE8, as as you said anything after the hash is never sent to the server, so I lose my parameters.

$location is breaking it, well the workaround created for IE8 is breaking it.
I understand my code change might be really specific and not wanted, so what about
being less specific.
Would you agree to have maybe just a callback function you can define on the $location, which $location will call before refreshing the page for the IE8 html5mode hashbang workaround?

Cheers,
Xavier

@IgorMinar
Copy link
Contributor

Why don't you store the token in the cookie and use that for authentication. your workaround uses cookies anyway.

@nolazybits
Copy link
Contributor Author

  1. I want to have minimal impact in my backend code of the angularjs IE8 workaround.
  2. with the previous code submitted. I was able to create cookie before IE8 refresh workaround and delete them after refresh as angularjs was in charge.
    Having the backend doing this is a problem:
    • browser detection with php is a bit cumbersome and raise false positives, + it goes against 1
    • In html5 compliant browsers, those cookie will stay in the browser (as no refresh as been done) -> this might cause problems.

I really think angularjs should give an option to run a function before refresh and after refresh for the IE8 workaround, as it is the one breaking the app. This should have actually been though about when the IE8 workaround as been designed/written.

I would be more than pleased to implement the pre / post refresh callback function and push it to update this pull request.

Thanks Igor.

Cheers,
Xavier

@nolazybits
Copy link
Contributor Author

Again I agree that this pull request was too specific, but I think having a pre/post refresh callback function would be the way to do it. Devs would then be able to do whatever they wish.

@IgorMinar
Copy link
Contributor

Since this is a problem only during bootstraping of the app (the redirect happens as one of the first things during bootstrap), you can just set the cookie via a script even outside of angular (before angular bootraps).

I really don't think that the callback you are requesting would do anything that isn't already possible via a modules' config function or via a regular script outside of angular.

@nolazybits
Copy link
Contributor Author

Would you be kind and share a bit of code implementing your solution please:

  • code called before IE8 html5mode workaround page refresh
  • code called once IE8 html5mode workaround page refresh has been done (ie page has refresh and application has been loaded)

I'm still new to AngularJS (working on and off with it for the past year) so I might have missed something. Cheers.

PS: didn't get notification of your answer. Sorry for the late reply

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants