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

Does CSV still get created even if automated Google Sheets options is set? #146

Open
gerardo-rodriguez opened this issue Jul 19, 2023 · 12 comments

Comments

@gerardo-rodriguez
Copy link
Member

I ran Lighthouse Parade with the --output google-sheets option.

I stepped away from my computer because an audit can take hours. I forgot to disable my laptop's sleep feature.

I then came back, and saw this error:

  FetchError: Invalid response body while trying to fetch https://sheets.googleapis.com/v4/spreadsheets/1QF4KmnIF6HYJ2Fdi4qZLV-87EpENzgblG_fMuYWwS_4/values/%27Lighthouse%20Results%27%21A409?valueInputOption=USER_ENTERED: read ECONNRESET

The Google Sheet was not being written to anymore.

How can we gracefully handle this use case?
Does a CSV still get created, or does the whole process stop?

An idea

At least a CSV gets created and some message/notification somewhere telling me along the lines of: "The Google Sheet write isn't working anymore, but no worries, you'll still get a CSV that you can [manually import](link to docs)".

@calebeby
Copy link
Member

@gerardo-rodriguez The --output option can be set multiple times, for example --output google-sheets --output something.csv. This could solve this use case if you wanted a backup.

If no --output is set, it defaults to a csv file. To answer the question in the title, no, if the output is set to google-sheets then it will not output to a csv file.

If your computer goes to sleep while running lighthouse-parade, then it won't be able to continue making new lighthouse reports, since doing that relies on an internet connection. I suspect that the data that it was trying to upload when that error came up was probably not useful data, the lighthouse data was likely not accurate if the computer was disconnected from the internet.

Maybe we could look into using something like this? https://www.npmjs.com/package/node-prevent-sleep (although it appears to only support windows).

I was looking into other options on npm and saw this 🙄 https://github.com/roldanjr/use-stay-awake that makes a react hook for this by loading and playing videos in the background 😂

Here's another option, macos only: https://github.com/nornagon/node-wake-lock

These are all a little sketchy to me since they all hook into native modules that call into OS API code. We can look into it more though, I don't think it's out of the question.

@gerardo-rodriguez
Copy link
Member Author

I was looking into other options on npm and saw this 🙄 https://github.com/roldanjr/use-stay-awake that makes a react hook for this by loading and playing videos in the background 😂

Wow. 😅

I suspect that the data that it was trying to upload when that error came up was probably not useful data, the lighthouse data was likely not accurate if the computer was disconnected from the internet.

Ah, that makes sense. So it seems a "stay awake" option might be the best and only option.

@calebeby We should at least update the docs and add a note/reminder saying "If you plan to leave your laptop running without supervision, ensure you disable sleep mode" or something like that. At least in the short term, this might be helpful.

@gerardo-rodriguez
Copy link
Member Author

The --output option can be set multiple times, for example --output google-sheets --output something.csv.

@calebeby This wasn't obvious to me when I read the next README docs. Let's add this to the docs as well. 🙂

@calebeby
Copy link
Member

The --output option can be set multiple times, for example --output google-sheets --output something.csv.

@calebeby This wasn't obvious to me when I read the next README docs. Let's add this to the docs as well. 🙂

@gerardo-rodriguez It already mentions it in the list of flags (from the --help text):

-o, --output The output file(s). CSV and Google Sheets are supported. Can be passed multiple times for multiple outputs. Example: -o cloudfour-a.csv -o google-sheets -o google-sheets:"Spreadsheet Name"

Is there somewhere else you were looking & expecting it that I could add it to?

@gerardo-rodriguez
Copy link
Member Author

It already mentions it in the list of flags (from the --help text):

-o, --output The output file(s). CSV and Google Sheets are supported. Can be passed multiple times for multiple outputs. Example: -o cloudfour-a.csv -o google-sheets -o google-sheets:"Spreadsheet Name"

Is there somewhere else you were looking & expecting it that I could add it to?

@calebeby LOL I guess the way the documentation is presented makes it harder to find. I didn't see it because it was horizontally scrolled out of view, even at larger viewports. Especially when the description is longer.

Maybe we can use a table to display that info? Something else?

Or, I don't know, @Paul-Hebert or @spaceninja, am I the only one that finds it harder to read the CLI options when presented as a fixed width horizontally scrolled view?

Screen Shot 2023-07-25 at 10 33 55 AM

@calebeby
Copy link
Member

I can switch that to be headings+paragraphs. The reason it is the way it is currently is because it makes it much easier to avoid the --help text diverging from the readme text when I change either (my process is: edit the --help text, then copy-paste the whole blob into the readme). But I can understand that headings+paragraphs would be easier to read so I'll switch it to that.

@gerardo-rodriguez
Copy link
Member Author

Thanks, @calebeby, that makes sense why it is the way it is. I appreciate you taking an extra step to make the docs more user-friendly. 🙌🏽

@gerardo-rodriguez
Copy link
Member Author

But I can understand that headings+paragraphs would be easier to read so I'll switch it to that.

@calebeby It sounds like you have an idea already, but I was playing with this to see how it felt. I was playing with adding a heading for each option section to make it super easy to link directly to a specific option.

If you like this approach, I can finish the rest and submit a PR if that helps since I already started editing the README. 🙂

Screen Shot 2023-07-25 at 11 03 25 AM

@calebeby
Copy link
Member

Yes, thanks, this is exactly what I'd had in mind! What do you think about using the flag itself as the heading? Like:

--max-crawl-depth

Control the maximum...

(I'm not overly attached to this idea)

@gerardo-rodriguez
Copy link
Member Author

@calebeby Oh, I think I like it better!

Screen Shot 2023-07-25 at 11 12 09 AM

Thanks for the suggestion! I'll submit a PR with these changes in a bit. 👍🏽

@calebeby
Copy link
Member

Thanks :shipit:

@gerardo-rodriguez
Copy link
Member Author

Here's the PR!

#148

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

No branches or pull requests

2 participants