Track unique history location states #14011

bcardarella · 2016-08-03T12:09:31Z

This PR will allow the HistoryLocation API to keep track of unique
state UUIDs for each state that is pushed/replaced. This change should not
break backwards compatibility and can allow for other libraries to use
this information to set proper scroll position.

The path alone does not provide enough information. For example, if you
visit page A, scroll down, then click on a link to page B, then click on
a link back to page A. Your actual browser history stack is [A, B, A].
Each of those nodes in the history should have their own unique scroll
position. In order to record this position we need an ID that is unique
for each node in the history.

rwjblue · 2016-08-03T17:12:48Z

packages/ember-routing/lib/location/history_location.js

@@ -150,7 +151,8 @@ export default EmberObject.extend({
   @param path {String}
  */
  pushState(path) {
-    let state = { path: path };
+    let id = guidFor(`${path}-${new Date()}`);


I don't think using guidFor on a string is very useful, if the idea is that all new states have a unique id I think this may be easier/better:

let state = { path, id: ++stateCounter };

yeah I figured that was hairy. I'll go about the stateCounter

bcardarella · 2016-08-03T18:57:06Z

@rwjblue updated

mixonic · 2016-08-08T00:53:38Z

packages/ember-routing/lib/location/history_location.js

@@ -33,6 +33,7 @@ export default EmberObject.extend({
    set(this, 'location', get(this, 'location') || window.location);

    this._popstateHandler = undefined;
+    this.stateCounter = 0;


This seems private, this._stateCounter would be preferable.

mixonic · 2016-08-08T00:56:39Z

@bcardarella this also adds public API to getState in the form of id, correct? Can you add some documentation to getState about the shape of the object being returned?

Generally seems reasonable.

mixonic · 2016-08-08T00:57:27Z

packages/ember-routing/lib/location/history_location.js

@@ -150,7 +151,8 @@ export default EmberObject.extend({
   @param path {String}
  */
  pushState(path) {
-    let state = { path: path };
+    let id = `${this.stateCounter++}`;


Can this just be the integer?

I feel like I had a good reason to force it to a string. Let me go back and see why I did that. It might not be necessary now.

This is something that I can handle upstream. Moving to just int

bcardarella · 2016-08-08T02:23:23Z

@mixonic updated per your suggestions

rwjblue · 2016-08-12T20:42:18Z

There is some question here if feature flagging and/or an RFC is needed here. I think the quick summary is that If this is introducing public API, then we should flag it (and potentially create a small and quick RFC for it)...

bcardarella · 2016-08-13T01:30:50Z

The counter itself is private as @mixonic suggested. The unique value added to the history states is not an API

rwjblue · 2016-08-13T01:39:41Z

OK, so no new public API is being added here then. c/d?

bcardarella · 2016-08-13T01:42:32Z

That's what I believe, but of course not my call ;)

mixonic · 2016-08-21T18:03:22Z

@bcardarella yeah the return value from a public API is of course at best public API itself, or at the worst intimate. For modern things that are properties but private API we've started using symbols and non-enumerable getters, but that obviously hasn't propagated through the whole codebase.

The use-case you described seems reasonable, but a socialized RFC would help gather other use cases that may be related. It would be good to hear from some other community members.

If you can draft something I'm happy to help edit and champion this with you 🙇

bcardarella · 2016-12-05T14:21:11Z

@mixonic finally got around to this: emberjs/rfcs#186

stefanpenner · 2016-12-06T03:52:01Z

thanks @bcardarella !

stefanpenner · 2016-12-06T04:32:27Z

After reading through the RFC and then commenting on it, i realized I believe this can already be implemented via map or WeakMap, but maybe we should talk about it on the RFC itself, no changes to ember required

krisselden · 2016-12-06T18:01:41Z

@stefanpenner session state is serialized for browser crash recovery including the history state, so if you don't map the id to session storage or the sequence your app will be broken if someone chooses to recover their session after a crash.

In fact this PR should really store the id sequence start in session storage.

bcardarella · 2016-12-06T18:03:40Z

@stefanpenner @krisselden should this conversation take place in the RFC?

krisselden · 2016-12-06T19:17:01Z

@bcardarella I'll copy & paste it

stefanpenner · 2016-12-07T00:54:38Z

@stefanpenner session state is serialized for browser crash recovery including the history state, so if you don't map the id to session storage or the sequence your app will be broken if someone chooses to recover their session after a crash.

In fact this PR should really store the id sequence start in session storage.

@krisselden ya, i came to this conclusion in the linked discussion.

homu · 2017-01-17T00:58:57Z

☔ The latest upstream changes (presumably #14821) made this pull request unmergeable. Please resolve the merge conflicts.

stefanpenner · 2017-01-17T01:15:28Z

packages/ember-routing/lib/location/history_location.js

@@ -150,7 +155,8 @@ export default EmberObject.extend({
   @param path {String}
  */
  pushState(path) {
-    let state = { path: path };
+    let id = this._stateCounter++;


We should update this to use a UUID more details here

bcardarella · 2017-01-17T02:21:47Z

@stefanpenner updated

rwjblue

The RFC for this was just merged (:tada:), so now this is ready for some more work.

Basic steps to land are:

add new feature to feature.json with null value
feature flag API changes (if (isFeatureEnabled('new-feature-name') { } etc)
feature flag tests

addressed

bcardarella · 2017-01-21T05:35:49Z

@rwjblue I wrapped the private scope _uuid function like so:

if (isFeatureEnabled('ember-unique-location-history-state')) {
  function _uuid() {
    return 'xxxxxxxx-xxxx-4xxx-yxxx-xxxxxxxxxxxx'.replace(/[xy]/g, function(c) {
      var r, v;
      r = Math.random() * 16 | 0;
      v = c === 'x' ? r : r & 3 | 8;
      return v.toString(16);
    });
  }
}

but am getting the following error:

18:3   error  Move function declaration to program root  no-inner-declarations

How should I resolve?

rwjblue · 2017-01-21T15:27:39Z

I think generally we do something like this:

let _uuid;
if (isFeatureEnabled('ember-unique-location-history-state')) {
  _uuid = function _uuid() { /*  implementation here */ };
}

bcardarella · 2017-01-21T18:40:22Z

All set

This PR will allow the HistoryLocation API to keep track of unique state IDs for each state that is pushed/replaced. This change should not break backwards compatibility and can allow for other libraries to use this information to set proper scroll position. The path alone does not provide enough information. For example, if you visit page A, scroll down, then click on a link to page B, then click on a link back to page A. Your actual browser history stack is [A, B, A]. Each of those nodes in the history should have their own unique scroll position. In order to record this position we need an ID that is unique for each node in the history. Moved from stateCounter to uuid Set under Feature Flag

Now that emberjs/ember.js#14011 has landed it would be best to align this library with the new implementation in anticipation of deprecating the RouterScroll for Ember 2.13 and eventually removing it in Ember 3.0.

knownasilya · 2017-08-02T17:01:05Z

Can this be used in 2.14 or is the addon by DollarShaveClub still necessary?

bcardarella · 2017-08-02T17:04:14Z

The addon is still necessary as it makes use of the token for the scroll positioning. If you have a use outside of the addon this functionality is in the HistoryLocation class in 2.14

bcardarella mentioned this pull request Aug 3, 2016

Enable backbutton for same routes DockYard/ember-router-scroll#14

Merged

rwjblue reviewed Aug 3, 2016
View reviewed changes

mixonic reviewed Aug 8, 2016
View reviewed changes

bcardarella mentioned this pull request Aug 11, 2016

router-scroll location type breaks non-root routes in development DockYard/ember-router-scroll#21

Closed

locks added the Needs Team Discussion label Nov 8, 2016

stefanpenner previously requested changes Jan 17, 2017

View reviewed changes

rwjblue removed the Needs Team Discussion label Jan 20, 2017

rwjblue requested changes Jan 20, 2017

View reviewed changes

rwjblue added Feature Needs Submitter Response labels Jan 20, 2017

rwjblue approved these changes Jan 21, 2017

View reviewed changes

rwjblue merged commit 13c26ed into emberjs:master Jan 21, 2017

bcardarella deleted the bc-unique-id-for-history-states branch January 21, 2017 19:47

bcardarella mentioned this pull request Jan 23, 2017

Align with Ember's HistoryLocation router DockYard/ember-router-scroll#38

Closed

briangonzalez mentioned this pull request Jan 24, 2017

Align with Ember's HistoryLocation router DockYard/ember-router-scroll#40

Merged

locks mentioned this pull request Mar 13, 2017

Ember 2.12 blog post emberjs/website#2824

Merged

18 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Track unique history location states #14011

Track unique history location states #14011

bcardarella commented Aug 3, 2016 •

edited

Loading

rwjblue Aug 3, 2016

bcardarella Aug 3, 2016

bcardarella commented Aug 3, 2016

mixonic Aug 8, 2016

bcardarella Aug 8, 2016

mixonic commented Aug 8, 2016

mixonic Aug 8, 2016

bcardarella Aug 8, 2016

bcardarella Aug 8, 2016

bcardarella commented Aug 8, 2016

rwjblue commented Aug 12, 2016

bcardarella commented Aug 13, 2016

rwjblue commented Aug 13, 2016

bcardarella commented Aug 13, 2016

mixonic commented Aug 21, 2016

bcardarella commented Dec 5, 2016

stefanpenner commented Dec 6, 2016

stefanpenner commented Dec 6, 2016 •

edited

Loading

krisselden commented Dec 6, 2016

bcardarella commented Dec 6, 2016

krisselden commented Dec 6, 2016

stefanpenner commented Dec 7, 2016

homu commented Jan 17, 2017

stefanpenner Jan 17, 2017

bcardarella commented Jan 17, 2017

rwjblue left a comment

bcardarella commented Jan 21, 2017

rwjblue commented Jan 21, 2017

bcardarella commented Jan 21, 2017

knownasilya commented Aug 2, 2017

bcardarella commented Aug 2, 2017

Track unique history location states #14011

Track unique history location states #14011

Conversation

bcardarella commented Aug 3, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcardarella commented Aug 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mixonic commented Aug 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcardarella commented Aug 8, 2016

rwjblue commented Aug 12, 2016

bcardarella commented Aug 13, 2016

rwjblue commented Aug 13, 2016

bcardarella commented Aug 13, 2016

mixonic commented Aug 21, 2016

bcardarella commented Dec 5, 2016

stefanpenner commented Dec 6, 2016

stefanpenner commented Dec 6, 2016 • edited Loading

krisselden commented Dec 6, 2016

bcardarella commented Dec 6, 2016

krisselden commented Dec 6, 2016

stefanpenner commented Dec 7, 2016

homu commented Jan 17, 2017

Choose a reason for hiding this comment

bcardarella commented Jan 17, 2017

rwjblue left a comment

Choose a reason for hiding this comment

bcardarella commented Jan 21, 2017

rwjblue commented Jan 21, 2017

bcardarella commented Jan 21, 2017

knownasilya commented Aug 2, 2017

bcardarella commented Aug 2, 2017

bcardarella commented Aug 3, 2016 •

edited

Loading

stefanpenner commented Dec 6, 2016 •

edited

Loading