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

make use of user timing api if available #998

Merged
merged 1 commit into from
Dec 11, 2016
Merged

Conversation

zspecza
Copy link
Contributor

@zspecza zspecza commented Dec 7, 2016

This introduces a client-side check for availability of the User Timing API and augments the genKey function to use performance.now instead of Date.now if the User Timing API is available.

The benefits of this change are:

  • the accuracy of the resulting _key is more precise in modern browsers (more specifically, instead of being a value that represents milliseconds since the unix epoch, it represents microseconds since the user started navigating - because microseconds are a smaller unit of measurement than milliseconds, this might generate more _keys)
  • mitigates a false negative on Lighthouse (giving apps that use vue-router a better score for being a progressive web app)

const genKey = () => String(Date.now())
// use User Timing api (if present) for more accurate key precision
let Time
if (typeof window !== 'undefined') {
Copy link
Member

Choose a reason for hiding this comment

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

You should import this from util/dom

import { isInBrowser } from '../util/dom'

We can also make it a const

const Time = isInBrowser ? (window.performance || Date) : Date

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@posva rebased with these changes :)

@posva
Copy link
Member

posva commented Dec 7, 2016

The result in the key is a bit different though. But it doesn't change anything.
Can you reference the false negative given by Lighthouse, please? It's mostly curiosity

@zspecza
Copy link
Contributor Author

zspecza commented Dec 7, 2016

The result in the key is a bit different though. But it doesn't change anything.

@posva Behavior will likely differ as performance.now outputs microseconds (since the user started navigation) rather than milliseconds (since the unix epoch), i.e. this change will probably generate a larger number of keys in modern browsers

Can you reference the false negative given by Lighthouse, please? It's mostly curiosity

AFAIK there is no way for me to link a reference due to the nature of the tool, so here's a screenshot of the offending rule:

screen shot 2016-12-07 at 7 48 44 pm

For a closer look, download the official chrome extension and run it on any site that uses vue-router (it works on localhost).

@posva
Copy link
Member

posva commented Dec 7, 2016

Screenshot is absolutely fine 👍 Thanks for sharing. Didn't know Lighthouse was checking that. Looks like a bit too much 😆

@zspecza
Copy link
Contributor Author

zspecza commented Dec 7, 2016

@posva I agree with you 100% - It's as if they've assumed performance.now() is a drop-in alternative to Date.now() for every use case. I shudder to think what users of moment.js will do about this :P

@posva
Copy link
Member

posva commented Dec 7, 2016

@fnlctrl @yyx990803 maybe you see an edge case I'm missing
Oh, I forgot to actually test if this removes the error

@posva
Copy link
Member

posva commented Dec 7, 2016

@declandewet Unfortunately this doesn't remove the Lighthouse false positive

edit: ofc its still there, there's a Date.now from somewhere else.
edit2: 👍

@zspecza
Copy link
Contributor Author

zspecza commented Dec 7, 2016

@posva - this removes the false positive on my end, must be one of your other dependencies :)

@posva
Copy link
Member

posva commented Dec 7, 2016

@declandewet Yes, I was testing with https://github.com/vuejs/vue-hackernews-2.0
the error disappears for the vendors but there're some Date usage in the app

@zspecza
Copy link
Contributor Author

zspecza commented Dec 7, 2016

@posva yeah - unfortunately there are valid use cases for Date over performance and it seems there were some concerns about this in the Lighthouse repo. I'm sure it will be addressed eventually.

The main benefit to focus on here is the one for accuracy - not the lighthouse audit :) since the getKey function is mostly (if not entirely) used in vue-router's scroll behaviour, consider it a progressively-enhanced micro-optimization for modern browsers that results in more accurate caching.

That being said, now that I think about it - it might be worthwhile to profile the performance of this. I see virtually no difference except the few millisecond overhead of checking for window, but it's worth having another pair of eyes looking into it

@fnlctrl
Copy link
Member

fnlctrl commented Dec 7, 2016

(random rant)
Tried the lighthouse thing out, though I don't really know if all this fuss over Progressive Web App actually make any sense..
I only use Date.now() to get unix timestamps, and I don't care about precision (as long as I get incremental values), yet it marked my website not modern just because of that.
Also, it puts Has a registered Service Worker on the top of the list. Wow, just wow. Seriously, since when did Google start recommending people something that's still a Working Draft? Service worker isn't even implemented by Safari/Edge yet.
It then suggested service workers can make your site work when offline. Hmm, interesting, but how? With Cache! ..and Guess what? Not supported by Safari/Edge.
Don't know if it's Safari being the new IE by lagging behind, or Chrome itself becoming the new IE by selling people those fancy new APIs when they're just Working Drafts.
(end of random rant)

@fnlctrl
Copy link
Member

fnlctrl commented Dec 7, 2016

@declandewet

The main benefit to focus on here is the one for accuracy - not the lighthouse audit :) since the getKey function is mostly (if not entirely) used in vue-router's scroll behaviour

Actually, accuracy isn't a concern here - Date.now() is only being used as unique keys. As long as Date.now() doesn't produce duplicates, it's fine.

@zspecza
Copy link
Contributor Author

zspecza commented Dec 7, 2016

I don't really know if all this fuss over Progressive Web App actually make any sense

@fnlctrl "Progressive Web App" is just a term used to describe a website that behaves like a mobile app using a combination of these technologies. When following these best practices, if a user visits your "progressive web app" a second time 5 minutes after their first visit, Chrome on Android will prompt them to install the website to the home screen and have it behave just like any other app on their phone. This removes the need for the user to 1. see the app advertised somewhere 2. search for the app in the play store and 3. download the app - it increases conversions for the site quite marginally. Obviously, there's a very real threat that PWAs might kill app stores - and the Apple App Store is too much of a cash cow for them to allow that to happen, unfortunately :( (I'm sure there are many other reasons too)

Actually, accuracy isn't a concern here - Date.now() is only being used as unique keys. As long as Date.now() doesn't produce duplicates, it's fine.

Date.now() can produce duplicates. It's a 1 in 1,000,000 chance, but it can happen. Perhaps a better solution then would be a uuid?

@fnlctrl
Copy link
Member

fnlctrl commented Dec 7, 2016

Well, for vue-router, Date.now() won't produce duplicates as long as the user doesn't trigger pushState(which triggers genKey) more than once in a ms. If that's what the user wants I think we can just let it happen...

I'm all for web apps killing native apps, but let's be realistic: unless the whole planet is dominated by Chrome and everyone stopped using iPhones (which uses Safari), it's just a waste of time trying to catch up with all these fancy new APIs that Chrome wants you to use. A prompt to install the website to the home screen? Sweet, but on Android only. Meh.

It's a good thing PWA sets up a standard for people to improve their website performance, but I don't appreciate it selling these immature APIs.

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.

None yet

4 participants