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

[BUGFIX beta] Preserve current history state on app boot #15223

Merged
merged 1 commit into from
Dec 20, 2017

Conversation

courajs
Copy link
Contributor

@courajs courajs commented May 10, 2017

RFC #186 introduced per-history-entry unique ids to the HistoryLocation.

But currently on app boot, a new uuid is always generated. This means that when refreshing the page, the same history entry is given a new uuid.

@courajs
Copy link
Contributor Author

courajs commented May 26, 2017

ping @stefanpenner

@courajs courajs changed the title [BUGFIX beta] Preserve current history state [BUGFIX beta] Preserve current history state on app boot May 26, 2017
@stefanpenner
Copy link
Member

@bcardarella mind taking a look, is this still in-spirit with the history state stuff you added?

@bcardarella
Copy link
Contributor

@courajs can you detail the use case? RFC #186 was intended to bring in the behavior originally build into ember-router-scroll https://github.com/dollarshaveclub/ember-router-scroll/blob/master/addon/locations/router-scroll.js

for the purposes of preserving scroll position between page transitions. If I hit a transition to a URL that is outside the Ember app then backbutton I am presented with the previous scroll position. From a behavior perspective it appears the history state is preserved and app boot doesn't interfere with it.

Can you give me an idea on how I would reproduce this outside of the unit test that you wrote? Perhaps I am overlooking an edge case.

@bcardarella
Copy link
Contributor

If there is a bug with this feature it would be great to get it fixed before 2.14 which I believe ships with the RFC #186 feature. That would be due June 5th.

@stefanpenner
Copy link
Member

stefanpenner commented May 26, 2017

Oh I see. This is to survive full hard page reloads? I think we explicitly decide not to support that due to complexities which I have now forgotten. Maybe @bcardarella remembers?

@bcardarella
Copy link
Contributor

If this is meant to support a hard refresh then I suspect we do want to support that use case. The goal was to create the behavior of a server-rendered page and its scroll state preservation.

With hard refresh as a use case you can see how an ember app with the router scroll addon behaves differently than other server rendered pages. We should strive to make the experience identical.

@courajs
Copy link
Contributor Author

courajs commented May 27, 2017

Yep, this is to support a hard refresh. I'm keeping a table in sessionStorage from uuid -> some other state, similar to scroll. Without this, after a hard reload, previous and future entries still work with the table, but the current one doesn't match up. This would help preserve page scroll when you refresh as well.

@courajs
Copy link
Contributor Author

courajs commented May 27, 2017

I looked through the RFC and original implementation and didn't see any mention of this - I think it was just an oversight.

@bcardarella
Copy link
Contributor

I think it was just an oversight

confirm

@bcardarella
Copy link
Contributor

@stefanpenner I'm 👍 on this change

@ef4
Copy link
Contributor

ef4 commented Dec 20, 2017

I reviewed this with @courajs and I'm going to merge it unless anybody has objections that haven't already been voiced in this thread.

@rwjblue
Copy link
Member

rwjblue commented Dec 20, 2017

@ef4 - Probably worth a rebase to make sure the new more strict linting rules and whatnot are not going to make master fail...

@ef4 ef4 merged commit cacefee into emberjs:master Dec 20, 2017
@courajs courajs deleted the stable-history-location branch February 23, 2018 01:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants