holidayextras / barometer

Framework agnostic browser performance metrics
MIT License
5 stars 5 forks source link

Expose onPageChange via window.barometer #23

Closed theninj4 closed 7 years ago

theninj4 commented 7 years ago

Resolves #16 . This will allow other applications to benefit from the algorithm to detect when a page transition has finished.

msaspence commented 7 years ago

I'm not sure about the name of the exposed function and there are conflicts but otherwise 👍

msaspence commented 7 years ago

Maybe onPageChangeIdle

msaspence commented 7 years ago

I know its not directly related to this PR but, 25ms for maxLag seems arbitrary add brittle. simple apps and/or powerful machines might not trigger this reliably. time out to not worry about it might be better.

theninj4 commented 7 years ago

I think it's the tense that seems slightly off. Javascript events trigger before their consequences are known - in this instance, we're triggering an event after the fact. How about it we change the tense from onPageChange (suggests the page is about to change) to onPageChanged (suggests the past has finished changing).

I'm keen to avoid mentioning idle - it may be how the page-change algorithm is currently implemented, but it's not what we're really measuring. If we were to change the algorithm to detect it differently, the naming would be wrong.

msaspence commented 7 years ago

What about the implementation though? Currently the durations set mean that this won't get fired on a bare minimum page.

theninj4 commented 7 years ago

If you were to drop barometer on a page that doesn't have any clientside JS, then no you won't get an onPageChanged event. You wouldn't get an onPageChangeIdle either. The README is clear on the definition of what we consider PageChanged to be.

msaspence commented 7 years ago

I'm just saying that definition seems like it might be brittle. Maybe we can make an improvement. The first thing I can think of is to say if it never got busy to just fire the event after 100 ms (or some other number) I think it's better to err on occasionally firing too early or too late than it is to occasionally not fire at all.

theninj4 commented 7 years ago

We can definitely reconsider the specifics once we get an idea of how this performs in production. I'd rather we ship what we need and come back for the bits we don't need at some point in the future.

msaspence commented 7 years ago

Thinking about this from the point of view of the bandwidth stats but I think this also applies @djbeaumont's request. I understand why you don't want to put Idle in the function name, but I think that is fundamentally what those two use cases want. "Tell me when the page is idle so that I can do the thing I need to do without impacting the page" they don't actually care about page changes. What we are after is actually quite different from the onPageChange function.

theninj4 commented 7 years ago

That's not really true - the browser will jump between idle and busy all the time - think about form validation, autocompletion, etc. We want an event that tells us that we've just navigated to a new page and everything has finished loading.

I discussed the requirements with Dave in person before he raised the issue, plus we're also a consumer of this functionality.

msaspence commented 7 years ago

The bandwidthStats' requirements don't align with that and, without having spoken to Dave, I'm not sure scripts that use the exposed function will either. They don't care about navigation, their concern is interfering with the main application. What if I hook into onPageChange for an app that doesn't change hash, url or push state? It will never get fired.

What I think we need is an onPageIdle which triggers when the lag is low enough, if the lag is already low, it will fire immediately. It will only ever fire the provided event once. This can be used to fire functions in the background in an attempt to not interfere with the application, regardless of navigation.

This is distinct from onPageChange which may use onPageIdle internally to workout we things have died down but is fundamentally different in that it is concerned about navigation events.

theninj4 commented 7 years ago

That's fine, the functionality you're after is only a couple of lines. It can be added in another pull request.