DBW: add audit for Date.now() usage #707

ebidel · 2016-09-26T22:52:59Z

R: @paulirish @brendankenny @GoogleChrome/lighthouse

samccone · 2016-09-26T23:35:07Z

TIL

brendankenny

Looking good so far

brendankenny · 2016-09-26T23:05:11Z

lighthouse-core/audits/dobetterweb/no-datenow.js

+      category: 'JavaScript',
+      name: 'no-datenow',
+      description: 'Site does not use Date.now() in its own scripts',
+      helpText: 'Consider using <a href="https://developer.mozilla.org/en-US/docs/Web/API/Performance/now" target="_blank">performance.now()</a>, which as better precision than <code>Date.now()</code> and always increases at a constant rate, independent of the system clock.',


FWIW, this does seem like one of those tests people are going to be annoyed about if they need Date.now() for an actual timestamp since the epoch or if performance.now() overhead isn't worth it

@brendankenny not annoyed, but if not Date.now(), what should be use if what you need is not performance? Maybe lighthouse should show recommendations besides just the warning/error?

brendankenny · 2016-09-26T23:06:37Z

lighthouse-core/gather/gatherers/dobetterweb/datenow.js

+
+  afterPass(options) {
+    return options.driver.evaluateAsync(`(${collectDateNowUsage.toString()}())`)
+        .then(errors => {


maybe something like dateNowUses to differentiate from the function below this handling actual errors?

brendankenny · 2016-09-26T23:22:17Z

lighthouse-core/audits/dobetterweb/no-datenow.js

+        err.url = `${JSON.stringify(err)}`;
+        prev.push(err);
+      }
+      return prev;


do you want to dedupe these so if a function calling Date.now() is itself called many times you don't get a long long list of them?

brendankenny · 2016-09-26T23:30:55Z

lighthouse-core/formatters/partials/url-list.html

@@ -15,7 +15,10 @@
    <summary>URLs</summary>
    {{#each this}}
      <div class="http-resource">
-        <span class="http-resource__url">{{this.url}}</span><span class="http-resource__protocol">({{this.protocol}})</span>
+        <span class="http-resource__url">{{this.url}}</span>


Shouldn't overload url here like this. Maybe just add a place for a more general text string that is styled the same?

brendankenny · 2016-09-26T23:31:13Z

lighthouse-core/audits/dobetterweb/no-datenow.js

+    // Filter out Date.now() usage from scripts on other domains.
+    const results = artifacts.DateNowUse.errors.reduce((prev, err) => {
+      if (url.parse(err.url).host === pageHost) {
+        err.url = `${JSON.stringify(err)}`;


you shouldn't modify the artifact itself here (and should pick a more descriptive property name once the formatter is changed)

brendankenny · 2016-09-26T23:39:42Z

Some idle thoughts while thinking about the next audit you (or whoever) write that uses this technique: it might be good to pull it up into driver, so that you any gatherer could evaluateScriptOnLoad a script that injected something and get events or a single event with lists of the locations where you did something.

It could also maybe be made more robust by throwing and catching errors but using the Debugger domain to turn on breaking on caught exceptions, then filtering out any not generated by Lighthouse, or maybe inserting debugger; statements and actually breaking there. The event/callback/whatever could be as simple as getting the callstack/location/whatever or eventually allowing more scripts to be executed at that point, the same way you can interact with the page in the DevTools console when execution is paused.

paulirish · 2016-09-26T23:41:44Z

lighthouse-core/gather/gatherers/dobetterweb/datenow.js

+    } catch (e) {
+      if (e.stack) {
+        const split = e.stack.split('\n');
+        const m = split[split.length - 1].match(/(https?:\/\/.*):(\d+):(\d+)/);


I tried out this date.now monkeypatch in the console, but it led to e.stack as

"Error: __called Date.now()__ at Error (native) at Function.Date.now (<anonymous>:4:13) at <anonymous>:1:6"

The match() then returns null. I think you could make the regex testing here a bit more liberal.

done with the ST API

paulirish · 2016-09-27T00:23:12Z

lighthouse-core/gather/gatherers/dobetterweb/datenow.js

+      throw Error('__called Date.now()__');
+    } catch (e) {
+      if (e.stack) {
+        const split = e.stack.split('\n');


HOLD UP WAIT A MINUTE

https://github.com/v8/v8/wiki/Stack-Trace-API#customizing-stack-traces

⬆️ ⬆️ ⬆️ ⬆️ ⬆️ ⬆️ ⬆️ ⬆️

ebidel · 2016-09-27T05:36:31Z

Well that looks useful!

On Mon, Sep 26, 2016, 5:23 PM Paul Irish notifications@github.com wrote:

@paulirish commented on this pull request.

In lighthouse-core/gather/gatherers/dobetterweb/datenow.js
#707 (review)
:
+/* global window, __returnResults */
+
+'use strict';
+
+const Gatherer = require('../gatherer');
+
+function patchDateNow() {
window.__stackTraceErrors = [];

// Override Date.now() so we know when if it's called.

const orig = Date.now;

Date.now = function() {

try {
 throw Error('**called Date.now()**');
} catch (e) {
 if (e.stack) {
   const split = e.stack.split('\n');
HOLD UP WAIT A MINUTE

https://github.com/v8/v8/wiki/Stack-Trace-API#customizing-stack-traces

⬆️ ⬆️ ⬆️ ⬆️ ⬆️ ⬆️ ⬆️ ⬆️

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#707 (review),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAOigAmLcglYHesjlR5fTGtZtEqN75Biks5quGHzgaJpZM4KHFKz
.

ebidel · 2016-09-27T16:11:12Z

Moved to the ST API. Works great. PTAL.

nicklepedde · 2016-12-27T15:18:37Z

I am one of those annoyed people, who does use and need Date.now(). My App has portions that require action from the customer by a specific date/time. Can we at least add an option to the report to ignore that rule?

wardpeet · 2016-12-27T15:55:16Z

you can create a custom configuration and don't include the audit & gatherer

ebidel added 4 commits September 26, 2016 17:51

DBW: add audit for Date.now() usage

5e92091

Remove @author

5f56cd1

url -> URL

e3df716

Update helpText

13ab3c1

brendankenny requested changes Sep 26, 2016

View reviewed changes

paulirish reviewed Sep 26, 2016

View reviewed changes

paulirish reviewed Sep 27, 2016

View reviewed changes

Use v8 Stack Trace API

62d8d96

brendankenny mentioned this pull request Sep 27, 2016

DBW: support source maps for JS audits #710

Closed

paulirish approved these changes Sep 27, 2016

View reviewed changes

paulirish merged commit 962948f into master Sep 27, 2016

paulirish deleted the datenow2 branch September 27, 2016 22:40

zspecza mentioned this pull request Dec 7, 2016

make use of user timing api if available vuejs/vue-router#998

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DBW: add audit for Date.now() usage #707

DBW: add audit for Date.now() usage #707

ebidel commented Sep 26, 2016 •

edited

samccone commented Sep 26, 2016

brendankenny left a comment

brendankenny Sep 26, 2016

davidcv5 Nov 22, 2016

brendankenny Sep 26, 2016 •

edited

ebidel Sep 27, 2016

brendankenny Sep 26, 2016

ebidel Sep 27, 2016

brendankenny Sep 26, 2016

ebidel Sep 27, 2016

brendankenny Sep 26, 2016 •

edited

ebidel Sep 27, 2016

brendankenny commented Sep 26, 2016

paulirish Sep 26, 2016

ebidel Sep 27, 2016

paulirish Sep 27, 2016

ebidel Sep 27, 2016

ebidel commented Sep 27, 2016

@paulirish commented on this pull request.

ebidel commented Sep 27, 2016

nicklepedde commented Dec 27, 2016

wardpeet commented Dec 27, 2016

DBW: add audit for Date.now() usage #707

DBW: add audit for Date.now() usage #707

Conversation

ebidel commented Sep 26, 2016 • edited

samccone commented Sep 26, 2016

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny Sep 26, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny Sep 26, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny commented Sep 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebidel commented Sep 27, 2016

@paulirish commented on this pull request.

ebidel commented Sep 27, 2016

nicklepedde commented Dec 27, 2016

wardpeet commented Dec 27, 2016

ebidel commented Sep 26, 2016 •

edited

brendankenny Sep 26, 2016 •

edited

brendankenny Sep 26, 2016 •

edited