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.48k stars 79 forks source link

Make extension background non-persistent #44

Closed bershanskiy closed 2 years ago

bershanskiy commented 3 years ago

Describe the bug Background page should not be persistent.

To Reproduce Steps to reproduce the behavior:

  1. Install extension in Chrome and close all dev tools, including Svelte.
  2. Open Chrome Task manager (click Shift+ESC).
  3. See "Extension: Svelte Devtools" in the process list, even though extension is not in use.

Expected behavior When extension is not in use, it should not consume computer resources (RAM).

Environment

Additional context If you are interested, I can file a PR to fix this. Right now, only Chrome supports non-persistent pages, but Firefox plans to add support soonTM.

Plan I will break up this into a few PRs. Here is a tentative plan (no particular order):

WIP (all these are blocked on already opened PRs):

  1. (TODO) Store and retreive Background context variables from chrome.storage.local when needed
  2. (TODO) Optimization: let background open and close persistent ports with DevTools only when needed
  3. (TODO) Declare background non-persistent in manifest.json and test it!

Review:

  1. 62

    Optimization: avoid chrome.tabs.onUpdated because it is triggered for every single other tab. Frequent callback runs restore background into active state unnecessarily.

  2. 60

Done:

  1. 54 Removes pagePorts in src/background.js

  2. 53 Fix: background has two handlers for 'startProfiler' (one is dead) and no handler for 'stopProfiler'

  3. 53 Get rid of gloal profilerEnabledList in src/background.js

    Value of this variable is set in DevTools tab and is used in content script. It makes sense to store it in either one, but not in background.

Edit: add plan!

RedHatter commented 3 years ago

I spent some time attempting to move to a non-persistent background script today but was unable to get it working.

If I'm understanding correctly in order to use a non-persistent background script correctly I would need to move the toolsPorts and pagePorts maps into chrome.storage.local but, unfortunately, these maps store message passing ports which are not serializable.

You are welcome to take a crack at it if you wish but I do not believe it will work with the current architecture.

bershanskiy commented 3 years ago

If I'm understanding correctly in order to use a non-persistent background script correctly I would need to move the toolsPorts and pagePorts maps into chrome.storage.local but, unfortunately, these maps store message passing ports which are not serializable.

Not exactly. Chrome persists the background context for as long as it has at least one open port, so you don't need to serialize ports. This persistence is both a blessing (ease of development) and a curse (efficiency loss), so I personally prefer avoiding ports and using messages.

I spent some time attempting to move to a non-persistent background script today but was unable to get it working.

What problem are you facing?

You are welcome to take a crack at it if you wish but I do not believe it will work with the current architecture.

I'd actually prefer just using regular message passing, if that's OK with you. I have a working local repo which works for me, can file a PR if you are interested.

RedHatter commented 3 years ago

If you have something working by all means create a pull request and I'll look at it.

bershanskiy commented 3 years ago

I didn't document my changes very well by making small commits, so the diffs are rateher long and not all changes are relevant. I'll split changes out in logical chuncks for simpler review and file incremental PRs. I'll make sure to reference this bug to help keep track.

RedHatter commented 3 years ago

Take your time. I appreciate the help.

bershanskiy commented 3 years ago

@RedHatter I have updated the top comment with a tentative plan. Feedback is welcome.

Edit: typo

bershanskiy commented 3 years ago

Technically, it would be fine to declare background non-persistent already (because background would be kept alive all the time when at least one Svelte DevTool tab is open), but I'd like to get #53 merged and do task 5 before that.

bershanskiy commented 3 years ago

@RedHatter I filed all the PRs which I could without depending on already existing PR. I still expect some PRs to have merge conflict which I can resolve after the first PR is merged. What do you think of what is already there?

bershanskiy commented 2 years ago

Closing as obsolete.