RFC: Track unique history location state #186

bcardarella · 2016-12-05T14:20:45Z

stefanpenner

Interesting approach! Don't let me comments sound too negative, hopefully we can flesh out the details so we can move forward on this.

stefanpenner · 2016-12-06T04:03:37Z

text/0000-track-unique-history-location-state.md

+Code: [PR#14011](https://github.com/emberjs/ember.js/pull/14011)
+
+We simply add a `stateCounter` so we can track uniqueness on two
+dimesions. Both `path` and the generated `id`.


dimesions -> dimensions

stefanpenner · 2016-12-06T04:05:20Z

text/0000-track-unique-history-location-state.md

+
+# Summary
+
+Track unique history location states


We should likely outline the public/intimate API that plugins (or consumers) will use to utilize this feature. This contract/protocol should be clearly defined, as it will be sticking around for some time.

stefanpenner · 2016-12-06T04:11:51Z

text/0000-track-unique-history-location-state.md

+
+Code: [PR#14011](https://github.com/emberjs/ember.js/pull/14011)
+
+We simply add a `stateCounter` so we can track uniqueness on two


the pr seems to add _stateCounternot stateCounter

_stateCounter seems abit strange, to provide an intentional private looking property as public API. What is the reasoning behind this?

where does this property live?

is it readOnly access or read/write property

@stefanpenner the private name was requested by @rwjblue

stefanpenner · 2016-12-06T04:12:07Z

text/0000-track-unique-history-location-state.md

+We simply add a `stateCounter` so we can track uniqueness on two
+dimesions. Both `path` and the generated `id`.
+
+This API will allow other libraries to reflect upon each location to


this seem motivation, not detailed design

stefanpenner · 2016-12-06T04:12:10Z

text/0000-track-unique-history-location-state.md

+is making use of a [modified `Ember.HistoryLocation` object to get this
+behavior](https://github.com/dollarshaveclub/ember-router-scroll/blob/master/addon/locations/router-scroll.js).
+
+Tracking unique state is required when setting the scroll position


this seem motivation, not detailed design

stefanpenner · 2016-12-06T04:14:15Z

text/0000-track-unique-history-location-state.md

+
+# Drawbacks
+
+None that I'm aware of


the property access is writable

the _ property queues private, but we want to enable add-ons to publicly use this, this seems like a teaching hazard.

no scroll position between tabs/sessions (likely fine though)

@stefanpenner the _stateCounter isn't meant to be accessed directly. The only thing that addon authors will have access to and should use is the { path: '/foo', id: 3 } object, in this case the id property being the new part of the object being proposed.

history state is like sessionStorage, it is serialized and tab specific, it is only restored on a browser crash recovery, the state counter should be stored in sessionStorage and incremented there.

@krisselden this would be ideal, do you have any resources I can read upon for this?

@krisselden reading https://developer.mozilla.org/en-US/docs/Web/API/Window/sessionStorage Its not clear to me on how to track unique page visits. If I visit A, then B, then A with a history stack of [A, B, A] how would sessionStorage API be used? Would the page be the key? If yes, then that only have a single dimension of uniqueness and doesn't account for multiple page visits. (which this RFC addresses)

If the entire history object is to be stored in sessionState, is this something that should be restored at app launch? That would seem like an odd user experience.

stefanpenner · 2016-12-06T04:15:29Z

text/0000-track-unique-history-location-state.md

+
+# Unresolved questions
+
+Is this public API? If so, would this be considered additive or


If it is intended for an addon to use, i believe it is public API.

stefanpenner · 2016-12-06T04:17:31Z

text/0000-track-unique-history-location-state.md

+  { path: '/about', id: 2 },
+  { path: '/', id: 1 }
+]
+```


if this is intended for add-on authors or custom location authors. We should likely describe in more detail how to teach them to use the feature.

I'm all for that, beyond the the id property being accessible on the history object there isn't anything else being proposed. IMO the easiest example would be to point towards ember-router-scroll's implementation

stefanpenner · 2016-12-06T04:19:53Z

text/0000-track-unique-history-location-state.md

+
+Code: [PR#14011](https://github.com/emberjs/ember.js/pull/14011)
+
+We simply add a `stateCounter` so we can track uniqueness on two


stateCounter i assume returns a number, do we wan to also guarantee that stateCounter is always incremented, such as counterFor(state1) < counterFor(state2) < counterFor(stateN)?

If it is guaranteed to be incrementing, is it incrementing for the life of the page, for the life of the app or for the life of the location. (this is more something i thought of, when thinking about how acceptances tests might interact with one another.

Alternatively, we can simply say it is a session unique id for a given application instance, and in practice it could be represented by a number, or a guid or....

or a guid

an early implementation of the code used Ember.guidFor however the problem is the only value available to generate the guid was the path and if you visit the same path but as different parts of the stack it will generate similar guids, we want unique values for each new visit.

GuidFor of the state object would assign a hidden ID to it, then forever that object would return that hidden id. Which seems like this would work, as all we are doing is assigning our own custom ID to state

guidFor() is also a counter, and history state restores when you reopen your browser.

stefanpenner · 2016-12-06T04:30:28Z

text/0000-track-unique-history-location-state.md

+
+# Alternatives
+
+The purpose for this behavior is to enable scroll position libraries.


If all we need is each state to have a unique identifier, could we just use a Map or WeakMap? I believe this may result in no public API change.

const scrollPositions = new Map() scrollPositions.set(state, [10, 10]); scrollPositions.set(state2, [100, 100]); scrollPositions.get(state) // => [10, 10] scrollPositions.get(state2) // => [100, 100]

If we are worried about a memory leak, a WeakMap could be used instead. Which ember actually has a polyfill that would work for this use case kicking around: https://github.com/emberjs/ember.js/blob/master/features.json#L6

The state being used by the history location is being set with replaceState https://www.w3.org/TR/2011/WD-html5-20110113/history.html#dom-history-replacestate

I believe it must be a pojo

I am not recommending we change the state object itself, But that we associate data (such as scroll position) with any given state pojo via map or weakmap.

This RFC seems to want to give unique identity to a given state pojo, so that other parts of the system can differentiate between them. As currently described, it suggests branding each pojo with an ID, but why brand at all if we can just use the objects own identity as a way to differentiate. A way to use an objects identity in such a way is a map, where one would use the state pojo as the key in the map. But that may make memory management tricky, hence recommending a weakmap instead.

The only thing I can think of that would prevent this from working, is if the state pojo we push onto the stack, isn't the same pojo (but still contains the same properties/values) we get when we pop off the stack.

Actually, i take that back. WeakMap wont work, because state is serialized/deserialized which means when popping it, we get a copy of the original state back. This means, object identity can't be used as key. So branding with something like __id__ or id as you proposed, seems like the correct path.

Sorry for taking you on an adventure, I did not realize that state object was serialized this way. In retrospect it makes sense.

@stefanpenner does that mean this review is satisfied?

bcardarella · 2017-01-09T21:43:05Z

@stefanpenner thanks! I do think there might be an outstanding issue however, one that I haven't tested. I think the use case of navigating out of the app then back-button'ing to the app. Considering that history state is being pushed on the navigator level I suspect the data and this RFC should be OK. I suspect the implementation in https://github.com/dollarshaveclub/ember-router-scroll needs to be updated to catch this when instantiating

stefanpenner · 2017-01-09T21:52:24Z

@bcardarella sorry for letting this linger. It slipped my mind.

Based on how you are serializing the state, I suspect that scenario should be fine. Although someone confirming it would be good.

stefanpenner · 2017-01-12T16:46:18Z

I'll be championing this RFC at this Fridays core team meeting.

bcardarella · 2017-01-12T17:01:44Z

🙌

nathanhammond · 2017-01-13T00:29:09Z

Can confirm that this works correctly with the history.state approach. (Was involved in this HTML spec.)

stefanpenner · 2017-01-13T19:25:46Z

@bcardarella one last thing, @krisselden pointed this one out during todays meeting. A monotonically increasing identifier wont work correctly, as it will result in duplicate ID's being serialized as the counter will be reset every time we reload the page. But the existing stored state objects will not be.

Instead of a monotonically incrementing counter, we could use the following uuid https://gist.github.com/lukemelia/9daf074b1b2dfebc0bd87552d0f6a537 snippet. Which will have sufficient entropy for the history state objects, and should mitigate the above concern.

With this change, we are comfortable moving this into FCP.

krisselden · 2017-01-13T20:01:23Z

Also, if you close and reopen, the tab is restored with its deserialized session. The counter will reset in this case even though the history states have been restored.

bcardarella · 2017-01-13T20:41:17Z

@stefanpenner @krisselden thank you for the feedback. Should I reference Luke's snippet directly as the suggested uuid solution or leave it ambiguous?

stefanpenner · 2017-01-13T22:08:27Z

leave it ambiguous?

Specifically, calling out that it requires a sufficient unique identifier to be resilient to the points @krisselden raised.

nathanhammond · 2017-01-14T01:03:56Z

We don't need a random ID for each time. One monotonically increasing identifier, and one generated GUID per runtime session. Store both in the history state object.

stefanpenner · 2017-01-15T21:46:37Z

We don't need a random ID for each time. One monotonically increasing identifier, and one generated GUID per runtime session. Store both in the history state object.

Both approaches would work, but its not clear to me what the benefit of having two separate values would be.

This RFC aims to provide a way to disambiguate between serialized / deserialized history state items, a UUID accomplishes this without implying ordering (I fear the monotonically increasing value my imply something we do not wish to guarantee). We should defer ordering to the history implementation.

rwjblue · 2017-01-20T19:42:54Z

Discussed again at today's Ember core team meeting. There have been no new issues brought up in the final comment period.

Landing....

nathanhammond · 2017-01-21T23:34:17Z

The only reason I would propose an incremental ID + GUID pair is for reasons of performance and "sufficient" randomness. Either works.

RFC: Track unique history location state

0ad4c13

bcardarella mentioned this pull request Dec 5, 2016

Track unique history location states emberjs/ember.js#14011

Merged

stefanpenner suggested changes Dec 6, 2016

View reviewed changes

Updated from feedback

c1fb4cc

stefanpenner approved these changes Jan 9, 2017

View reviewed changes

Change from counter to uuid per Stef's request

455ff5f

stefanpenner added the Final Comment Period label Jan 17, 2017

rwjblue merged commit b2f10c5 into emberjs:master Jan 20, 2017

bcardarella deleted the features/bc/track-unique-history-location-state branch January 20, 2017 19:43

courajs mentioned this pull request May 10, 2017

Restore unique history location state #227

Closed

myartsev mentioned this pull request May 24, 2017

Is this addon still needed in Ember 2.13? DockYard/ember-router-scroll#54

Closed


		Code: [PR#14011](https://github.com/emberjs/ember.js/pull/14011)

		We simply add a `stateCounter` so we can track uniqueness on two


		# Unresolved questions

		Is this public API? If so, would this be considered additive or


		# Alternatives

		The purpose for this behavior is to enable scroll position libraries.

RFC: Track unique history location state #186

RFC: Track unique history location state #186

Conversation

bcardarella commented Dec 5, 2016 • edited Loading

stefanpenner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcardarella Dec 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefanpenner Dec 6, 2016 • edited Loading

Choose a reason for hiding this comment

stefanpenner Dec 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcardarella commented Jan 9, 2017

stefanpenner commented Jan 9, 2017 • edited Loading

stefanpenner commented Jan 12, 2017

bcardarella commented Jan 12, 2017

nathanhammond commented Jan 13, 2017 • edited Loading

stefanpenner commented Jan 13, 2017 • edited Loading

krisselden commented Jan 13, 2017

bcardarella commented Jan 13, 2017

stefanpenner commented Jan 13, 2017

nathanhammond commented Jan 14, 2017 • edited Loading

stefanpenner commented Jan 15, 2017

rwjblue commented Jan 20, 2017

nathanhammond commented Jan 21, 2017

bcardarella commented Dec 5, 2016 •

edited

Loading

bcardarella Dec 6, 2016 •

edited

Loading

stefanpenner Dec 6, 2016 •

edited

Loading

stefanpenner Dec 6, 2016 •

edited

Loading

stefanpenner commented Jan 9, 2017 •

edited

Loading

nathanhammond commented Jan 13, 2017 •

edited

Loading

stefanpenner commented Jan 13, 2017 •

edited

Loading

nathanhammond commented Jan 14, 2017 •

edited

Loading