Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: Track unique history location state #186

Merged
merged 3 commits into from
Jan 20, 2017
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
93 changes: 93 additions & 0 deletions text/0000-track-unique-history-location-state.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
- Start Date: 2016/12/05
- RFC PR: (leave this empty)
- Ember Issue: (leave this empty)

# Summary

Track unique history location states
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


# Motivation

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.

# Detailed design

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stefanpenner the private name was requested by @rwjblue

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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....

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

dimesions. Both `path` and the generated `id`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dimesions -> dimensions


This API will allow other libraries to reflect upon each location to
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seem motivation, not detailed design

determine unique state. For example,
[ember-router-scroll](https://github.com/dollarshaveclub/ember-router-scroll)
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seem motivation, not detailed design

properly based upon where you are in the history stack, as described in
[Motivation](#motivation)

# How We Teach This

We could describe what meta data is generated for each location in the
history stack. For example, it could look like:

```js
// visit page A

[
{ path: '/', id: 1 }
]

// visit page B

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

// visit page A

[
{ path: '/', id: 3 },
{ path: '/about', id: 2 },
{ path: '/', id: 1 }
]

// click back button

[
{ path: '/about', id: 2 },
{ path: '/', id: 1 }
]
```
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


# Drawbacks

None that I'm aware of
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 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)

Copy link
Contributor Author

@bcardarella bcardarella Dec 6, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.


# Alternatives

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@stefanpenner stefanpenner Dec 6, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@stefanpenner stefanpenner Dec 6, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stefanpenner does that mean this review is satisfied?

There are two other solutions in the wild. One is in the guides that
suggests resetting the scroll position to `(0, 0)` on each new route
entry. The other is
[ember-memory-scroll](https://github.com/ef4/memory-scroll) which I
believe is better suited for tracking scroll positions for components
rather than the current page.

However, in both cases neither solution provides the experience that
users have come to expect from server-rendered pages. The browser tracks
scroll position and restores it when you revisit the page in the history
stack. The scroll position is unique even if you have multiple instances
of the same page in the stack.

# Unresolved questions

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

breaking?