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

[ui/globalState] do reactionary fetch in the next tick #6788

Closed
wants to merge 1 commit into from

Conversation

spalger
Copy link
Contributor

@spalger spalger commented Apr 5, 2016

In order to react to the url changing we listen for the $routeUpdate event on the $rootScope. This event is fired as a part of the transition process before the $location service has flushed it's changes to the browser. Because of this, any changes that happen in the same tick are merged together as a single change. This is generally desirable, but since we are using $location.replace() pretty liberally when the State detects that it has been removed from the URL we need to actually do this in the next tick. This way the $location.replace() call will never be merged with the location change that triggered the update.

This will fix the weird back-button behavior seen when using a link that doesn't have the global state written to it (basically anything that's not a tab or in the nav).

In order to react to the url changing we listen for the $routeUpdate event on the $rootScope. This event is fired as a part of the transition process before the $location service has flushed it's changes to the browser. Because of this, any changes that happen in the same tick are merged together as a single change. This is generally desirable, but since we are using $location.replace() pretty liberally when the State detects that it has been removed from the URL we need to actually do this in the next tick. This way the $location.replace() call will never be merged with the location change that triggered the update.

This will fix the weird back-button behavior seen when using a link that doesn't have the global state written to it (basically anything that's not a tab or in the nav).
@Bargs
Copy link
Contributor

Bargs commented Apr 6, 2016

So I don't know much about $evalAsync but the docs for it say:

The $evalAsync makes no guarantees as to when the expression will be executed

Can you point me to any documentation that says $evalAsync is guaranteed to execute in the next digest cycle?

Also, can you explain why we're using $location.replace() if the new location has no query params? I don't understand the purpose of that logic in the first place.

@Bargs Bargs assigned spalger and unassigned Bargs Apr 6, 2016
@spalger
Copy link
Contributor Author

spalger commented Apr 7, 2016

Yeah, I guess I misread the last point in those docs... hmm... let me think about this.

can you explain why we're using $location.replace() if the new location has no query params?

We do this because the url needs to have the state written to it, or unexpected changes occur and the "previous" can get lost (by a refresh for instance). The first approach we thought about was copying the state into every link (and keeping those links up to date) but decided it was too complicated and likely to fail due to neglect.

Instead we decided that any time the state service received a "$routeUpdate" and the state had been wiped out it would be assumed that the state needed to be added back after the fact. In order to prevent multiple history events we decided we would use $location.replace() pretty liberally, which shouldn't have any effect in just about any scenario (that we know of)

@Bargs
Copy link
Contributor

Bargs commented Apr 7, 2016

Hmmmm yeah I see what you're saying now, removing the replace gets you into an infinite loop with the back button since a querystring-less link creates two history entries. Hmmmmm... I'll ponder this as well

@spalger
Copy link
Contributor Author

spalger commented May 2, 2016

Closed in favor of #7076

@spalger spalger deleted the fix/backButtonSettings branch October 18, 2019 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants