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

feat($location): add support for state and title in pushState #3325

Closed
wants to merge 1 commit into from
Closed

feat($location): add support for state and title in pushState #3325

wants to merge 1 commit into from

Conversation

mgol
Copy link
Member

@mgol mgol commented Jul 24, 2013

Adds $location pushState & replaceState methods acting as proxies to history
pushState & replaceState methods. This allows using pushState to change state
and title as well as URL. Note that these methods are not compatible with
browsers not supporting the HTML5 History API, namely IE<10.

This builds on work from #1675. I applied changes based on developers' remarks, I added a few tests and fixed a couple of issues regarding the older pull request.

As for the fallback story - there isn't any easy fallback for browsers not supporting the HTML5 History API. It's possible to use a shim like https://github.com/browserstate/history.js which declares the History objects normalizing some stuff or to adjust it to just polyfill the history pushState and replaceState methods as well as the state property, though it would require quite significant amount of code.

I'll repeat what I wrote in a comment in the older pull request thread: would it be possible to add this feature while making note IE9 and older are not supported with this methods? I personally work more & more on projects dumping IE8 and older completely and IE9 already has way lower market share than IE8 and it's going quickly down as most IE9 users are auto-updated to IE10.

This should work for Google projects as well because Google policy is to fully support only the current and previous versions of each browser and once IE11 goes out in a few months, this means dumping IE9 which makes pushState available in all supported browsers.

I'm commited to make this patch go inside Angular, our team needs it a lot. If you have any remarks/need more tests/etc., I'll apply the needed changes.

@petebacondarwin
Copy link
Member

PR Checklist (Minor Feature)

  • Contributor signed CLA now or in the past (if you just signed, leave a comment here with your real name for cross reference)
  • Feature improves existing core functionality
  • API is compatible with existing Angular apis and relevant standards (if applicable)
  • PR doesn't contain a breaking change
  • PR contains unit tests
  • PR contains e2e tests (if suitable)
  • PR contains documentation update
  • PR passes all tests on Travis (sanity)
  • PR passes all tests on ci.angularjs.org (cross-browser compatibility)
  • PR is rebased against recent master
  • PR is squashed into one commit per logical change
  • PR's commit messages are descriptive and allows us to autogenerate release notes (required commit message format)
  • All changes requested in review have been implemented

@mgol
Copy link
Member Author

mgol commented Jul 27, 2013

@petebacondarwin All unit tests are passed locally, there's some problem with Travis that it can't find the Closure compiler...

It used to work before, I've just rebased a moment ago to the current master.

EDIT: it failed again but on some animation-related stuff, I didn't touch this code. Maybe current master is broken?

@mgol
Copy link
Member Author

mgol commented Jul 27, 2013

I corrected the error (current master build is broken) so Travis tests now pass.

@jeffbcross
Copy link
Contributor

@mzgol, I reviewed this with @IgorMinar and @ksheedlo. The feature is good, but it can't be merged in yet. It will have to wait until we only support browsers which support pushState, or until we can implement a proper fallback story. A good fallback story would have significant implications on various services, and would probably be implemented similarly to history.js. For now, I've scheduled the PR for the post-1.2 milestone so we can revisit it later.

Also, don't worry about making Travis happy right now; it's flaky.

@mgol
Copy link
Member Author

mgol commented Aug 7, 2013

@jeffbcross @IgorMinar @ksheedlo, thank you all for the review.

I'm eager to get this functionality into upstream so maybe I could find time to create a history.js-inspired polyfill for state handling. As I wrote earlier, though, it would be rather large so I wanted to get everything straight first. A couple of questions:

  1. Would you accept such a polyfill to the main codebase?

  2. Since the size would be large, how would you like to proceed? Would patching $browser and some pieces in $location in core and adding $location.pushState/replaceState methods together with adding a polyfill only in a separate module work? Do you have any other ideas?

  3. If this was created as a separate module, would it be acceptable for this module to be only IE10+ compatible? (it would still require patching $browser and $location in core but Angular users wouldn't be able to make use of changed parts without the module.

  4. Since pushState is only supported in IE10+ (other browsers were way faster), is it possible that this pull request be merged in a foreseeable future? IE8 seems still to keep strong, much more than IE9, probably due to Windows XP users.

  5. Any other ideas what could I do to make this patch land in the upstream Angular repository?

Thank you for your attention.

@IgorMinar
Copy link
Contributor

one of the things I'd like to investigate post 1.2 is to remove a bunch of code that internally proxies browser apis to make them api compatible. This approach worked fine for us for a while, but it's quite clear that we could now do way better by using polyfills (e.g. ES5 polyfill) and just expect the modern api to be there.

this strategy goes along the way of what is needed for the html5 history apis.

in the other news, post 1.2 we'll likely drop IE8 and maybe even IE9 support (if IE11 release date is confirmed for this September). That would make the situation even easier for most of us.

@mgol
Copy link
Member Author

mgol commented Aug 8, 2013

@IgorMinar Thanks for the info! That's great news. I'll keep this pull request alive & updated so that it's easier to get it to core post-1.2.

@mgol
Copy link
Member Author

mgol commented Aug 8, 2013

There's one problem with the polyfill approach to the History API. Differently than most APIs, the History API needs to work with the forward/back browser buttons. Since the only situation when a back button doesn't cause a refresh in browsers not supporting the History API is to use hashes, there would have to be some mechanism in Angular to ignore a certain part of the hash for uses other than state-related (e.g. something like #somehash&$$state=base64encodedstate). It can be done; it won't be automatic like with other polyfills, though.

@mgol
Copy link
Member Author

mgol commented Dec 15, 2013

Hmm, is the milestone correct here? I'd love to see it in 1.2.x but since it supports IE8 & IE9 and my patch introduce methods that won't work there I was under the impression it wasn't going to be merged yet... The same for 1.3.x since it doesn't drop IE9. Did sb mis-triage? @jeffbcross @IgorMinar @petebacondarwin.

@tamtakoe
Copy link

This feature very important for us too. We whant to save scroll position in state objects.

@mgol
Copy link
Member Author

mgol commented Dec 20, 2013

@tamtakoe You need to explicitely +1 it for the bot to parse the comment and count it as a vote.

@tamtakoe
Copy link

👍

4 similar comments
@jrencz
Copy link

jrencz commented Dec 23, 2013

+1

@sicarrots
Copy link

+1

@jerzydem
Copy link

+1

@cnjsstong
Copy link

+1

@mgol
Copy link
Member Author

mgol commented Feb 26, 2014

For anyone following this PR: I'll keep rebasing this PR onto subsequent Angular stable releases until it gets merged (not before 2.0, though since it requires dropping IE9) so if you need it, you can use EE/angular.js#pushState

Adds $location pushState & replaceState methods acting as proxies to history
pushState & replaceState methods. This allows using pushState to change state
and title as well as URL. Note that these methods are not compatible with
browsers not supporting the HTML5 History API, namely IE<10.

Closes #3325
@jeffbcross
Copy link
Contributor

Given that we decided not to drop IE9 support for Angular 1.x, I'm going to close this issue.

Angular 2 will certainly support pushState. Since Angular 2 is a complete rewrite, this PR would not apply.

@jeffbcross jeffbcross closed this Mar 10, 2014
@mgol
Copy link
Member Author

mgol commented Mar 12, 2014

Sure, @jeffbcross. I'll keep rebasing this branch to subsequent betas & 1.3.x releases since we need it anyway; maybe sb else will use it as well.

@mgol
Copy link
Member Author

mgol commented Apr 26, 2014

Rebased over 1.3.0-beta.7.

mgol added a commit to mgol/angular.js that referenced this pull request Sep 11, 2014
Adds $location pushState & replaceState methods acting as proxies to history
pushState & replaceState methods. This allows using pushState to change state
and title as well as URL. Note that these methods are not compatible with
browsers not supporting the HTML5 History API, e.g. IE 9.

Closes angular#3325
@mgol
Copy link
Member Author

mgol commented Sep 11, 2014

For the continuation of work on this PR see #9027.

@mgol
Copy link
Member Author

mgol commented Oct 8, 2014

PR #9027 has landed in 6fd36de, you can now use state in Angular (API changed from what was proposed in this PR, though).

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.

None yet

9 participants