Skip to content
This repository was 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
mgol wants to merge 1 commit into
angular:masterfrom
EE:pushState
Closed

mgol wants to merge 1 commit into
angular:masterfrom
EE:pushState

Conversation

@mgol

@mgol mgol commented Jul 24, 2013

Copy link
Copy Markdown
Member

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
Copy Markdown
Contributor

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

mgol commented Jul 27, 2013

Copy link
Copy Markdown
Member Author

@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

mgol commented Jul 27, 2013

Copy link
Copy Markdown
Member Author

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

@jeffbcross

Copy link
Copy Markdown
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

mgol commented Aug 7, 2013

Copy link
Copy Markdown
Member Author

@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
Copy Markdown
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

mgol commented Aug 8, 2013

Copy link
Copy Markdown
Member Author

@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

mgol commented Aug 8, 2013

Copy link
Copy Markdown
Member Author

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

mgol commented Dec 15, 2013

Copy link
Copy Markdown
Member Author

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
Copy Markdown

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

@mgol

mgol commented Dec 20, 2013

Copy link
Copy Markdown
Member Author

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

@tamtakoe

Copy link
Copy Markdown

👍

4 similar comments
@jrencz

jrencz commented Dec 23, 2013

Copy link
Copy Markdown

+1

@sicarrots

Copy link
Copy Markdown

+1

@jerzydem

Copy link
Copy Markdown

+1

@cnjsstong

Copy link
Copy Markdown

+1

@mgol

mgol commented Feb 26, 2014

Copy link
Copy Markdown
Member Author

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
Copy Markdown
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

mgol commented Mar 12, 2014

Copy link
Copy Markdown
Member Author

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

mgol commented Apr 26, 2014

Copy link
Copy Markdown
Member Author

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

mgol commented Sep 11, 2014

Copy link
Copy Markdown
Member Author

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

@mgol

mgol commented Oct 8, 2014

Copy link
Copy Markdown
Member Author

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.

9 participants