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

Warn in DevTools before clearing localStorage #772

Closed
1j01 opened this issue Oct 13, 2016 · 20 comments
Closed

Warn in DevTools before clearing localStorage #772

1j01 opened this issue Oct 13, 2016 · 20 comments
Assignees

Comments

@1j01
Copy link

1j01 commented Oct 13, 2016

I tried out the lighthouse Chrome extension and it wiped out my IndexedDB etc. without any indication that it was going to do so. I can see why it would, but this deleted a lot of data that could have been important. So it should mention that.

@brendankenny brendankenny changed the title Extension clears all storage without asking Extension should warn that it will clear all storage Nov 4, 2016
@michaeljonsampson
Copy link

Just ran into this. A warning would be a good addition.

@paulirish
Copy link
Member

@michaeljonsampson curious.. which part of storage was cleared that you were surprised by? We actually keep cookies alone, but it's possible you were using localStorage for similar purposes. :/

@michaeljonsampson
Copy link

IndexedDB data was deleted.

@paulirish
Copy link
Member

Let's stop clearing idb and localstorage. Leave it to the user to clear that themselves.

@paulirish paulirish changed the title Extension should warn that it will clear all storage Cease clearing localStorage Mar 9, 2018
@brendankenny
Copy link
Member

If we do this, I think we should detect if the site has something in idb or localstorage and issue a top-level warning that it should be manually cleared if it might impact loading time. e.g. predownloaded or precomputed assets (like compiled wasm modules).

It would be easy to forget to clear between lighthouse runs and have no idea why your score changed so much, especially if the caching is done by a library and without code written by the developer themselves.

@wardpeet
Copy link
Collaborator

wardpeet commented Sep 3, 2019

The extension will be replaced with a different version by #9193

Devtools has a checkbox to not clear storage:
image

Whenever we fix #2599 we can integrate multiple checkboxes in devtools but as of now, This issue can be tracked by #2599 as well.

@wardpeet wardpeet closed this as completed Sep 3, 2019
@Aditya94A
Copy link

It seems like the checkbox was removed. Is there some other way to do this now?

image

@patrickhulce
Copy link
Collaborator

@AdityaAnand1 it's behind the "Settings" cog in the top right now.

image

@1j01
Copy link
Author

1j01 commented Apr 13, 2020

If it's hidden behind a cog, it's no longer signaling to user that it's going to clear storage.
"Generate report" sounds very additive and friendly, but it's a destructive action.

For context, in my case I'm storing documents for a drawing program in IndexedDB via localforage.
Luckily I had saved most of the documents I wanted to keep, but this was very surprising.
I'm developing this app with an ethos of "don't destroy data by default" (and more generally, "fearless exploration"), so I'm saving undo/redo history, as a tree, with the document. I care a lot about this stuff.

@patrickhulce
Copy link
Collaborator

Yeah a fair point @1j01! Feel free to star the Chromium issue to add a warning (https://bugs.chromium.org/p/chromium/issues/detail?id=806488)

@connorjclark connorjclark reopened this Apr 13, 2020
@connorjclark connorjclark changed the title Cease clearing localStorage Warn in DevTools before clearing localStorage Apr 13, 2020
@paulirish
Copy link
Member

proposal:

  • new "Clear caches" checkbox which does sw, cache storage, appcache, but NOT localstorage, idb, and websql
  • we look at the localstorage/idb/websql and if they have stuff in there (for that origin), then we tell them they should do incognito, etc.

@paulirish
Copy link
Member

@connorjclark can you capture here what you were proposing for this?

@connorjclark
Copy link
Collaborator

just this

we look at the localstorage/idb/websql and if they have stuff in there (for that origin), then we tell them they should do incognito, etc.

we should do this for other things too. a list of warnings below "Generate Report" sayings things like, FYI: We noticed local overrides, we noticed data in storage, we noticed blocked requests, just FYI in case this wasn't intentional.

@brendankenny
Copy link
Member

Reading this DevTools use of Chrome Profiles doc, can we avoid having to make these assumptions and reduce magic if we open an incognito-ish new window when "Clear storage" is checked, and reuse the tab when it isn't checked?

Right now (from the doc) it looks like you can do Target.createBrowserContext to get an off the record BrowserContextID, then pass that into Target.createTarget to create the new window. From that doc it sounds like there is some work to be done to make it a really real ephemeral profile that the rest of Chrome will work with normally, but it seems like it would work once that's done?

I assume this would be more straightforward to do in the DevTools side of things than in Lighthouse core, but I wonder if it would make anything in interoperation with chrome-launcher easier as well.

@mpkelly
Copy link

mpkelly commented Jun 10, 2020

Please deselect "Clear storage" by default or at least show me a prompt that tells me you're going to wipe out my DB.

@connorjclark connorjclark removed their assignment Aug 26, 2020
@adamraine
Copy link
Member

@brendankenny looks like the ephemeral profiles idea was scrapped.

I think adding a warning is the best option for now.

@adamraine
Copy link
Member

lh1

This is a basic solution I came up with. Currently checks for localstorage/idb/websql via Storage.getUsageAndQuota.

I'm temporarily using the "unauditable" error text. Looks like there are a couple ways we could display the warning:

  • A warning list below the "Generate report" button: Similar to the picture. This would still be pretty obvious but not impossible to miss. Might be better for other warnings too @connorjclark
  • An explicit confirmation prompt: This would be impossible to miss, not sure if it's the best way to display an error list.
  • Combination of both: Detailed warning list with confirmation prompt that just says something like "There are some warnings, pls check before running"

I'm currently in favor of just the confirmation prompt for now but I'm willing to hear others' thoughts.

@connorjclark
Copy link
Collaborator

Combination of both

That would be interesting! But with an additional feature of "Never remind me again" (we could add it to the settings cog, I guess).

Other warnings we might want in this list:

  • Local Overrides is on–Lighthouse will audit the page with your overrides
  • Blocked Urls–Lighthouse will audit the page but some requests will be blocked
  • more?

@kepta
Copy link

kepta commented Sep 13, 2020

This is a pretty bad experience. I lost a lot of data by not knowing that running a lighthouse report will clear it. I took a backup of that data in one of my other website, but apparently I ran lighthouse on that too, so now I lost it forever.

devtools-bot pushed a commit to ChromeDevTools/devtools-frontend that referenced this issue Oct 8, 2020
Lighthouse users are sometimes unaware that auditing a page can clear
potentially important resources:
GoogleChrome/lighthouse#772

Lighthouse will no longer clear resources in IndexedDB, WebSQL, or
local storage when auditing a page. Since that change may affect
performance scores, Lighthouse will display a top level warning in the
report when data was found in any of those three locations. The warning
tells the user they should audit in an incognito window.

This CL adds an additional warning to the Lighthouse DevTools panel
which conveys the same information.

Preview:
https://imgur.com/nN1ZfFR

Bug: 806488
Change-Id: I64fd9e197868f690af0e0b89de52e0b7baa3f1fa
Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/2385182
Commit-Queue: Adam Raine <asraine@google.com>
Reviewed-by: Connor Clark <cjamcl@chromium.org>
Reviewed-by: Peter Marshall <petermarshall@chromium.org>
babot pushed a commit to binaryage/dirac that referenced this issue Oct 9, 2020
Lighthouse users are sometimes unaware that auditing a page can clear
potentially important resources:
GoogleChrome/lighthouse#772

Lighthouse will no longer clear resources in IndexedDB, WebSQL, or
local storage when auditing a page. Since that change may affect
performance scores, Lighthouse will display a top level warning in the
report when data was found in any of those three locations. The warning
tells the user they should audit in an incognito window.

This CL adds an additional warning to the Lighthouse DevTools panel
which conveys the same information.

Preview:
https://imgur.com/nN1ZfFR

Bug: 806488
Change-Id: I64fd9e197868f690af0e0b89de52e0b7baa3f1fa
Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/2385182
Commit-Queue: Adam Raine <asraine@google.com>
Reviewed-by: Connor Clark <cjamcl@chromium.org>
Reviewed-by: Peter Marshall <petermarshall@chromium.org>
@patrickhulce
Copy link
Collaborator

@adamraine has taken care of us here 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests