sveltejs / svelte-devtools

A browser extension to inspect Svelte application by extending your browser devtools capabilities
https://chromewebstore.google.com/detail/svelte-devtools/kfidecgcdjjfpeckbblhmfkhmlgecoff
MIT License
1.45k stars 77 forks source link

Store profiler enabled/disabled state in inspected window's sessionStorage #53

Closed bershanskiy closed 3 years ago

bershanskiy commented 3 years ago

I decided to split out this fix from other refactoring PRs because it might be harder to review refactoring PRs which change behavior than to review those that don't.

bershanskiy commented 3 years ago

Jut to clarify: I'm not sure how to test this change, so I'm not sure how to check whether extension was performing as desired before and that this did not break anything.

bershanskiy commented 3 years ago

This fix is incomplete, since there are multiople bugs. I think it makes more sense to get rid of profilerEnabledList in this contexdt entirely rather than fix it here.

RedHatter commented 3 years ago

Without profilerEnabledList the profiler will not stay enabled over page reloads meaning the initial rendering of the Svelte app can't be profiled.

bershanskiy commented 3 years ago

As far as I understand, enabled/disabled state of profiler for a specific tab originates from profiler button on DevTools page and that value is stored in profilerEnabled. I have a draft patch which just takes value directly from profilerEnabled and propagates it all the way to content script without ever storing it in background context. I'm pretty sure this is the best way to do it. If you really want to keep profilerEnabled on background context and keep current logic, I can put it into chrome.storage.local (tab IDs are just numbers and they are easily serializable).

RedHatter commented 3 years ago

That might work fine. Early in the development process I had an issue where the content script would clear the value, I don't remember the details, but that was a while ago and may no longer be applicable.

bershanskiy commented 3 years ago

Instead of using window.profilerEnabled I switched to window.sessionStorage.SvelteDevToolsProfilerEnabled which persists during page refreshes an at the same time is isolated per frame and gets cleared out after tab close. This simplifies code, fixes handling of stopProfiler message, and gets rid of one more global on background.

RedHatter commented 3 years ago

This change seems to have broken the profiler. Switch to profiler, refresh the page, no events are displayed.

RedHatter commented 3 years ago

Nevermind, it seems to be working now. Was probably just an instance of the occasional instability SvelteDevTools deals with due to sveltejs/svelte#5994

Edit: No, sorry, I was on the wrong branch. It is still not working as per my comment above.

bershanskiy commented 3 years ago

Switch to profiler, refresh the page, no events are displayed.

I know what I did wrong: last minute right before comitting I changed unload event handler to delete this setting. Of course, that's wrong and I'll remove this. I'm not sure what's the best way to clean up sessionStorage when tools are poen, profiling is is enabled and and user navigates away from the page. Technically, leaving that value there shouldn't cause any issues, since it will be overwritten before use.

bershanskiy commented 3 years ago

I have fixed the issue and tested that it works for me now.

Diff since last review:

--- a/rollup.config.js
+++ b/rollup.config.js
@@ -85,10 +85,7 @@ export default [{
     e => e.source == window && sendMessage(e.data),
     false
   )
-  window.addEventListener('unload', () => {
-    delete window.sessionStorage.SvelteDevToolsProfilerEnabled
-    sendMessage({ type: 'clear' })
-  })
+  window.addEventListener('unload', () => sendMessage({ type: 'clear' }))
 }`
   },
   plugins: [ resolve() ]