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

currentchange is not suitable for measuring the time #59

Closed
domenic opened this issue Mar 5, 2021 · 3 comments · Fixed by #125
Closed

currentchange is not suitable for measuring the time #59

domenic opened this issue Mar 5, 2021 · 3 comments · Fixed by #125

Comments

@domenic
Copy link
Collaborator

domenic commented Mar 5, 2021

After #46, appHistory.current updates synchronously, and thus currentchange fires synchronously, for all single-page navigations. So the example in the explainer which uses currentchange to measure the time is totally wrong.

This should probably be solved together with #14 and #33 but I wanted to note it separately since it's a serious bug in the explainer.

@tbondwilkinson
Copy link
Contributor

A counter proposal I think would be a afternavigate (or navigateend or whatever) event, that is sent whenever a navigation is fully finished, that would have largely the same properties as the currentchange event but would have the added benefit of only firing after a navigation had fully settled. I think if we go this route, we would get rid of currentchange altogether, and changing the timing of the event I think actually makes it more broadly useful (since it's not just identical to navigate but without the respondWith capability).

@frehner
Copy link

frehner commented Mar 30, 2021

Was talking with someone and the confusion around the timing of currentchange and the differences to navigate, so I remembered this issue.

Regarding a new event - would it just be easier to just add the timing information to navigatesuccess - since that event fires when respondWith() promises complete (unless it's been aborted), right?

@domenic
Copy link
Collaborator Author

domenic commented Mar 30, 2021

Yeah, navigatesuccess has the appropriate timing for that. However per #33 we might want to expose the timing information in a completely different way; we'll see...

domenic added a commit that referenced this issue May 27, 2021
This replaces our previous broken idea of using currentchange for this. Closes #59. Closes #33. #14 remains to track whether currentchange is actually useful.
domenic added a commit that referenced this issue Jun 4, 2021
This replaces our previous broken idea of using currentchange for duration tracking. Closes #59. Closes #33. Closes #14 by removing currentchange entirely for now (we can always add it back later if we see a use case).
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 a pull request may close this issue.

3 participants