Feature/add devtools viewer link to sheets #131
Conversation
* Update readme * Show `chromeFlags` as a usable feature flag * Add `options` example * Update readme to show lighthouse flag support
…ols_viewer_link_to_sheets
|
Hi. Thanks for the contribution. cc @paulirish @pedro93 , maybe @samccone |
lib/sheets/index.ts
Outdated
| @@ -50,6 +51,12 @@ class Sheets { | |||
| results.forEach(data => { | |||
| const getTiming = (key: string) => data.timings.find(t => t.id === key).timing; | |||
| const dateObj = new Date(data.generatedTime); | |||
| let viewerUrl = ''; | |||
|
|
|||
| if(typeof data.fileName !== 'undefined' && typeof data.fileName !== 'undefined') { | |||
There was a problem hiding this comment.
if(data.fileName) {
viewerUrl = VIEWER_URL_PREFIX + data.fileId;
}There was a problem hiding this comment.
Huh - just noticed one of those is supposed to be data.fileId, but that's the only one needed now - I originally intended to use both, but ditched the idea of using the file name as the link text.
I use typeof rather than a truthy check as it's won't choke on undefined.
There was a problem hiding this comment.
So you don't need to be concerned of that because undefined is false when type checked in a boolean scenario.
`data.fileName` not used now (was considering making it the link text)
|
@denar90 Ah good point - I hadn't considered a case where someone uploaded to drive with out submitting to sheets! Well spotted. |
There was a problem hiding this comment.
I have this idea.
First I'd rather don't mix sheets with uploading to gdrive, it's a bit different modules.
What I think is extending MetricsResults interface with one more field which is viewerUrl. It will be optional.
So if upload flag is passed then condition where MetricsResults data and viewerUrl will be mixed should be somewhere here - https://github.com/paulirish/pwmetrics/blob/master/lib/index.ts#L201
Hense when appending results you will be able to do smth like data.viewerUrl
https://github.com/paulirish/pwmetrics/pull/131/files#diff-fe52226e90803c8fafc2594b79b63be5R73
One more thing. We should also mention about this feature in docs.
|
@denar90 That makes sense - it's certainly tidier. And yes, documentation is handy! I've got some urgent work today, I'll take a look at this later. (I've switched accounts btw - the comments before were from my work account on my previous contract) |
…s_viewer_link_to_sheets
Strip out uneeded changes from prior commits on this branch. Add `configIsSubmitAndUpload()` to lib/index.ts Pass that fn to Sheets class. Add null value `viwerUrl` to Metrics class return object to prevent undefined type errors in Sheets paulirish#131
DisasterMan78
force-pushed
the
feature/add_devtools_viewer_link_to_sheets
branch
from
22e2558 to
587c95e
Compare
Strip out uneeded changes from prior commits on this branch. Add `configIsSubmitAndUpload()` to lib/index.ts Pass that fn to Sheets class. Add null value `viwerUrl` to Metrics class return object to prevent undefined type errors in Sheets paulirish#131
Strip out uneeded changes from prior commits on this branch. Add `configIsSubmitAndUpload()` to lib/index.ts Pass that fn to Sheets class. Add null value `viwerUrl` to Metrics class return object to prevent undefined type errors in Sheets Update readme.md paulirish#131
DisasterMan78
force-pushed
the
feature/add_devtools_viewer_link_to_sheets
branch
from
587c95e to
a3bd4c7
Compare
|
@denar90 Excuse the delay, life likes to interfere. I've created a function I've just done it for now, but I don't like it much. Overall it's much more concise and tidy though. Any suggestions? Also, I haven't been able to properly check the viewer link as the viewer refuses to acknowledge it's authorised to access my drive account. Grr... |
|
@DisasterMan78 sorry for so big delay. I'll take a look at it on the upcoming weekend. |
| @@ -96,7 +96,7 @@ class PWMetrics { | |||
| console.log(messages.getMessage('MEDIAN_RUN')); | |||
| this.displayOutput(results.median); | |||
| } else if (this.flags.submit) { | |||
| const sheets = new Sheets(this.sheets, this.clientSecret); | |||
| const sheets = new Sheets(this.sheets, this.clientSecret, this.configIsSubmitAndUpload); | |||
There was a problem hiding this comment.
What if not pass function to the sheets but extend results.runs with viewerUrl like
const sheets = new Sheets(this.sheets, this.clientSecret, this.configIsSubmitAndUpload);
await sheets.appendResults(extendResultsToSubmit(results.runs));
....
//extendResultsToSubmit method
return results.map(data => {
if (this.flags.submit && this.flags.upload) {
data.viewerUrl = getTimelineViewerUrl(data.fileId);
}
return data;
});
in this case we can cut off some stuff
wdyt?
|
Quick update on this: This is still on my list to return to when I find the time, but if anyone would like to have a crack at it in the meantime, feel free. |
If files are uploaded to drive, then a link to chrome devtools timeline viewer which opens the given file is added to comment column of google sheets file.
Requires app authorisation.
This is a quick and dirty hack - am open to advice for improvements or how to write tests for it, or am happy for it to be considered a feature request.
Currently there is no way to connect a sheets entry and a file - files are identified by a unix timestamp which is not present in the sheet, and even if it were, cross-referencing would be tedious.
This provides a simple and expedient way of viewing the detail for any test that has had data uploaded.