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.

Sign up for GitHub

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

Jump to bottom

Rename and update semantics for navigation duration measurement #27

Merged
merged 2 commits into from
Feb 11, 2021
Merged

Rename and update semantics for navigation duration measurement #27

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7708a42
Rename and update semantics for navigationStart(TimeStamp)
domenic Feb 5, 2021
7b51341
startTime; point to 33
domenic Feb 11, 2021
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
17 changes: 11 additions & 6 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -550,12 +550,14 @@ This can be useful for cleaning up any information in secondary stores, such as

The `window.appHistory` object has an event, `currententrychange`, which allows the application to react to any updates to the `appHistory.currentEntry` property. This includes both navigations that change its value, and calls to `appHistory.updateCurrentEntry()` that change its `state` or `url` properties. This cannot be intercepted or canceled, as it occurs after the navigation has already happened; it's just an after-the-fact notification.

This event has one special property, `event.navigationStartTimeStamp`, which for same-document navigations gives the value of `event.timeStamp` for the corresponding `navigate` event. This allows it to be used for determining the overall load time, including the time it took for a promise passed to `e.respondWith()` to settle:
This event has one special property, `event.startTime`, which for [same-document](#appendix-types-of-navigations) navigations gives the value of `performance.now()` when the navigation was initiated. This includes for navigations that were originally [cross-document](#appendix-types-of-navigations), like the user clicking on `<a href="https://example.com/another-page">`, but were transformed into same-document navigations by [navigation interception](#navigation-monitoring-and-interception). For completely cross-document navigations, `startTime` will be `null`.
Copy link

Choose a reason for hiding this comment

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

s/includes for navigations/includes navigations/?


"Initiated" means either when the corresponding API was called (like `location.href` or `appHistory.pushNewEntry()`), or when the user activated the corresponding `<a>` element, or submitted the corresponding `<form>`. This allows it to be used for determining the overall time from navigation initiation to navigation completion, including the time it took for a promise passed to `e.respondWith()` to settle:

```js
appHistory.addEventListener("currententrychange", e => {
if (e.navigationStartTimeStamp) {
const loadTime = e.timeStamp - e.navigationStartTimeStamp;
if (e.startTime) {
const loadTime = e.timeStamp - e.startTime;

document.querySelector("#status-bar").textContent = `Loaded in ${loadTime} ms!`;
analyticsPackage.sendEvent("single-page-app-nav", { loadTime });
Expand All @@ -565,7 +567,9 @@ appHistory.addEventListener("currententrychange", e => {
});
```

_TODO: could we populate this for cross-document navigations too? Then it kind of overlaps with existing timing APIs, and is probably a lot harder to implement..._
_TODO: reconsider cross-document navigations. There will only be one (the initial load of the page); should we even fire this event in that case? (That's [#31](https://github.com/WICG/app-history/issues/31).) Could we give `startTime` a useful value there, if we do?_

_TODO: is this property-on-the-event design the right one, or should we integrate with the performance timeline APIs, or...? Discuss in [#33](https://github.com/WICG/app-history/issues/33)._
Copy link

Choose a reason for hiding this comment

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

We should totally integrate this with the performance APIs, but that doesn't necessarily mean this timestamp is not needed. Are there other use cases for this timestamp, other than reporting?
e.g. I know that we have Event.timestamp, which is awfully similar.


_TODO: Add a non-analytics examples, similar to how people use `popstate` today._

Expand Down Expand Up @@ -802,6 +806,7 @@ This proposal is based on [an earlier revision](https://github.com/slightlyoff/h
Thanks also to
[@chrishtr](https://github.com/chrishtr),
[@dvoytenko](https://github.com/dvoytenko),
[@esprehn](https://github.com/esprehn),
[@housseindjirdeh](https://github.com/housseindjirdeh),
[@jakearchibald](https://github.com/jakearchibald), and
[@slightlyoff](https://github.com/slightlyoff)
Expand Down Expand Up @@ -895,10 +900,10 @@ dictionary AppHistoryUpcomingNavigateEventInit : EventInit {
interface AppHistoryCurrentEntryChangeEvent : Event {
constructor(DOMString type, optional AppHistoryCurrentEntryChangeEventInit eventInit = {});

readonly attribute DOMHighResTimeStamp? navigationStartTimeStamp;
readonly attribute DOMHighResTimeStamp? startTime;
};

dictionary AppHistoryCurrentEntryChangeEventInit : EventInit {
DOMHighResTimeStamp? navigationStartTimeStamp = null;
DOMHighResTimeStamp? startTime = null;
};
```