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

Content script is reloaded twice when the file is saved. #938

Closed
5 tasks done
kazu-ux opened this issue Aug 28, 2024 · 30 comments
Closed
5 tasks done

Content script is reloaded twice when the file is saved. #938

kazu-ux opened this issue Aug 28, 2024 · 30 comments
Labels
bug Something isn't working

Comments

@kazu-ux
Copy link

kazu-ux commented Aug 28, 2024

Describe the bug

Content script is reloaded twice when the file is saved.

Reproduction

wxt-video01.mp4

Steps to reproduce

Run npm install followed by npm run dev

System Info

System:
    OS: Windows 10 10.0.19045
    CPU: (8) x64 AMD Ryzen 5 3400G with Radeon Vega Graphics 

    Memory: 1.61 GB / 13.94 GB
  Binaries:
    Node: 20.11.1 - C:\Program Files\nodejs\node.EXE
    Yarn: 1.22.22 - C:\Program Files\nodejs\yarn.CMD
    npm: 10.2.4 - C:\Program Files\nodejs\npm.CMD
    pnpm: 9.9.0 - C:\Program Files\nodejs\pnpm.CMD
    bun: 1.1.10 - ~\AppData\Local\reflex\bun\bin\bun.EXE     
    Watchman: 20230806.083715.0 - D:\watchman\bin\watchman.EXE
  Browsers:
    Edge: Chromium (127.0.2651.74)
    Internet Explorer: 11.0.19041.4355
  npmPackages:
    wxt: ^0.19.7 => 0.19.7

Used Package Manager

npm

Validations

@kazu-ux kazu-ux added the pending-triage Someone (usually a maintainer) needs to look into this to see if it's a bug label Aug 28, 2024
@aklinker1
Copy link
Collaborator

aklinker1 commented Aug 29, 2024

Can't reproduce:

Video
output.mp4

But I think I can guess what the problem is. Can you apply this patch and share the logs?

diff --git a/node_modules/wxt/dist/core/create-server.mjs b/node_modules/wxt/dist/core/create-server.mjs
index 30688b19352a3c5df0a99ae3526f92952d375cd3..7d3db3668d23d0cecdd2db550901185b5bb0c861 100644
--- a/node_modules/wxt/dist/core/create-server.mjs
+++ b/node_modules/wxt/dist/core/create-server.mjs
@@ -102,6 +102,7 @@ function createFileReloader(server) {
     changeQueue.push([event, path]);
     await fileChangedMutex.runExclusive(async () => {
       if (server.currentOutput == null) return;
+      console.log(changeQueue);
       const fileChanges = changeQueue.splice(0, changeQueue.length).map(([_, file]) => file);
       if (fileChanges.length === 0) return;
       await wxt.reloadConfig();

@aklinker1 aklinker1 added bug Something isn't working and removed pending-triage Someone (usually a maintainer) needs to look into this to see if it's a bug labels Aug 29, 2024
@kazu-ux
Copy link
Author

kazu-ux commented Aug 30, 2024

I applied the patch.

wxt-video04.mp4

I noticed that the Content script works correctly when I update the code using Notepad.

wxt-video03.mp4

@aklinker1
Copy link
Collaborator

aklinker1 commented Aug 30, 2024

Hmm, not what I expected. It seems like VS Code is saving the file multiple times, and slow enough that my current debouncing logic isn't considering it a single save.

On my side, the only thing I could do is increase the delay, or replace the current logic with a real throttle. But I haven't seen anyone else with this error, so I'd like to figure out why it's happening to you first.

Do you have any extensions installed that would be causing this? Can you restart vscode and open it with extensions disabled to see if that's the problem?

code --disable-extensions path/to/your-project

@kazu-ux
Copy link
Author

kazu-ux commented Aug 30, 2024

I tried creating a new profile and disabling extensions, but the issue still persists.

wxt-video05.Clipchamp.mp4

@1natsu172
Copy link
Contributor

1natsu172 commented Aug 30, 2024

Hmm, I was able to reproduce this. Sometimes I got multiple “changed” logs. It happens sometimes when saved(⌘+S) once every 3sec by feel. Doesn't seem to be caused by personal something tool, it occurs in both npm and pnpm with VSCode and TextEdit.app
image

@1natsu172
Copy link
Contributor

1natsu172 commented Aug 30, 2024

Nah, if I look closely at…it's a bit different from his. He has [Changed: -> Reloaded:] x3, but I have multiple Changed: and Reloaded: is not shown after that. 🤷

Edit: He also gets multiple browser alerts, but I only get 1 So I couldn't reproduce.

@1natsu172
Copy link
Contributor

@kazu-ux The difference between our environment and yours is that Windows/MacOS and you are using different disk drives C:&D: (I noticed it).

And I found the Windows drive issue in Chokidar paulmillr/chokidar#665 - does the problem continue when you run WXT on the C: drive?

@kazu-ux
Copy link
Author

kazu-ux commented Sep 7, 2024

The same issue occurred on the C drive.

@1natsu172
Copy link
Contributor

@kazu-ux Okay, once you said that it does not reproduce in Notepad, is it 100% ? If so, we can say the problem isn't with chokidar. Because in the video you posted with the code --disable-extensions, the problem isn't happening at “Hello content. 2” and the timing is causing the problem at “Hello content. 3”. So the problem may in fact occur with Notepad if you try it many times.

@kazu-ux
Copy link
Author

kazu-ux commented Sep 7, 2024

Yes, I edited the code 50 times in Notepad, but no issues occurred.

@1natsu172
Copy link
Contributor

@kazu-ux Thanks for many tried. So…it may as I thought, be a VSCode issue or that with the VSCode's integrated terminal. We can only suspect VSCode settings, but what happens if you try set .output dir to the Files: Watcher Exclude and disable to TypeScript > TsServer > Experimental: Use Vs Code Watcher with workspace settings?

You could add the below code to .vscode/settings.json, or set up in the GUI.

{
  "files.watcherExclude": {
    "**/.output/**": true
  },
  "typescript.tsserver.experimental.useVsCodeWatcher": false
}

I can only think that something primitive in VSCode is the cause.

@kazu-ux
Copy link
Author

kazu-ux commented Sep 9, 2024

I changed settings. but the issue still persists.

I noticed that it works properly when I edit the popup code.

wxt-video06.Clipchamp.mp4

@1natsu172
Copy link
Contributor

So weird, reloading popup and content-script have different methods, but I don't know why it would make a difference with that and the editor. 🤷

@aklinker1
Copy link
Collaborator

aklinker1 commented Sep 10, 2024

Even if we can't reproduce, the issue is quite clear. There's a couple of solutions:

  1. Leave it as-is, queuing up file changes that occur while rebuilding
  2. Debounce the file saves by ~100ms (or longer) so if a second save happens in that timeframe, it doesn't trigger a rebuild
  3. Ignore all file changes until the build is finished

Thoughts on option 2? That's my preferred solution

@1natsu172
Copy link
Contributor

@aklinker1 Yes, I think option 2 is the best if we try to tune it up. Chokidar has many performance options, but too complicated lol. https://github.com/paulmillr/chokidar#performance

Do we use awaitWriteFinish or debounce on the WXT side to the extent that performance does not degrade? Or, we can't cover all environments, so provide a watcher performance options with an wxt.config.ts.

@aklinker1
Copy link
Collaborator

Didn't know about that option... Could be a solution as well.

The other option is to wait for the v4 rewrite they're in the middle of.

You can already pass in the watcher options via vite - WXT doesn't have its own file watcher

@aklinker1
Copy link
Collaborator

You can already pass in the watcher options via vite - WXT doesn't have its own file watcher

Actually, that might be wrong. I need to double check the code

@1natsu172
Copy link
Contributor

Actually, that might be wrong. I need to double check the code

Yep, we can actually pass it via vite config. But currently the user side values isn't merged with the default WXT side values in getBaseConfig. So it's fragile now.

const getBaseConfig = async (baseConfigOptions?: {

The other option is to wait for the v4 rewrite they're in the middle of.

I was thinking too. I just don't know if v4 will change the performance aspect. Looking at the current v4 branch, it looks like the main focus is TSing and getting rid of dependencies.

And vite is discussing about watcher with chokidar for a long time lol. vitejs/vite#13593

@1natsu172
Copy link
Contributor

Maybe we should follow the chokidar v4 and vite movement and only ready the stable passing watcher option in place until then.

@aklinker1
Copy link
Collaborator

Yeah, I'm good with that. For now, let's just add debouncing

@1natsu172
Copy link
Contributor

1natsu172 commented Sep 12, 2024

let's just add debouncing

Let's do so. I can work on it, but do you have any particulars about the debounce library?

@aklinker1
Copy link
Collaborator

aklinker1 commented Sep 12, 2024

@1natsu172 thanks!

I prefer unjs packages: https://unjs.io/packages/perfect-debounce

But if there's already another as a sub dependency, we can use that one too

@1natsu172
Copy link
Contributor

@kazu-ux We are working on PR for this issue. For reference, we would like to know how many seconds the reload actually takes, so please help us out.

The following patch will give you the seconds for reloading.

diff --git a/packages/wxt/dist/core/create-server.mjs b/packages/wxt/dist/core/create-server.mjs
index d74a536..239f099 100644
--- a/packages/wxt/dist/core/create-server.mjs
+++ b/packages/wxt/dist/core/create-server.mjs
@@ -19,6 +19,7 @@ import {
   getContentScriptJs,
   mapWxtOptionsToRegisteredContentScript
 } from "./utils/content-scripts.mjs";
+import { formatDuration } from "./utils/time.mjs";
 export async function createServer(inlineConfig) {
   await registerWxt("serve", inlineConfig, async (config) => {
     const { port, hostname } = config.dev.server;
@@ -92,7 +93,16 @@ export async function createServer(inlineConfig) {
     reloadContentScripts(server.currentOutput.steps, server);
   });
   const reloadOnChange = createFileReloader(server);
-  server.watcher.on("all", reloadOnChange);
+  let prev = Date.now();
+  const timerlog = () => {
+    const now = Date.now();
+    console.log(formatDuration(now - prev));
+    prev = now;
+  };
+  server.watcher.on("all", (...args) => {
+    timerlog();
+    reloadOnChange(...args);
+  });
   return server;
 }
 function createFileReloader(server) {

@pnd280
Copy link

pnd280 commented Sep 14, 2024

@kazu-ux We are working on PR for this issue. For reference, we would like to know how many seconds the reload actually takes, so please help us out.

The following patch will give you the seconds for reloading.

diff --git a/packages/wxt/dist/core/create-server.mjs b/packages/wxt/dist/core/create-server.mjs
index d74a536..239f099 100644
--- a/packages/wxt/dist/core/create-server.mjs
+++ b/packages/wxt/dist/core/create-server.mjs
@@ -19,6 +19,7 @@ import {
   getContentScriptJs,
   mapWxtOptionsToRegisteredContentScript
 } from "./utils/content-scripts.mjs";
+import { formatDuration } from "./utils/time.mjs";
 export async function createServer(inlineConfig) {
   await registerWxt("serve", inlineConfig, async (config) => {
     const { port, hostname } = config.dev.server;
@@ -92,7 +93,16 @@ export async function createServer(inlineConfig) {
     reloadContentScripts(server.currentOutput.steps, server);
   });
   const reloadOnChange = createFileReloader(server);
-  server.watcher.on("all", reloadOnChange);
+  let prev = Date.now();
+  const timerlog = () => {
+    const now = Date.now();
+    console.log(formatDuration(now - prev));
+    prev = now;
+  };
+  server.watcher.on("all", (...args) => {
+    timerlog();
+    reloadOnChange(...args);
+  });
   return server;
 }
 function createFileReloader(server) {

image
Highlighted regions are double reloads
btw, disable trailing seems to fix the issue 21446b1

@kazu-ux
Copy link
Author

kazu-ux commented Sep 14, 2024

@kazu-ux We are working on PR for this issue. For reference, we would like to know how many seconds the reload actually takes, so please help us out.

The following patch will give you the seconds for reloading.

diff --git a/packages/wxt/dist/core/create-server.mjs b/packages/wxt/dist/core/create-server.mjs
index d74a536..239f099 100644
--- a/packages/wxt/dist/core/create-server.mjs
+++ b/packages/wxt/dist/core/create-server.mjs
@@ -19,6 +19,7 @@ import {
   getContentScriptJs,
   mapWxtOptionsToRegisteredContentScript
 } from "./utils/content-scripts.mjs";
+import { formatDuration } from "./utils/time.mjs";
 export async function createServer(inlineConfig) {
   await registerWxt("serve", inlineConfig, async (config) => {
     const { port, hostname } = config.dev.server;
@@ -92,7 +93,16 @@ export async function createServer(inlineConfig) {
     reloadContentScripts(server.currentOutput.steps, server);
   });
   const reloadOnChange = createFileReloader(server);
-  server.watcher.on("all", reloadOnChange);
+  let prev = Date.now();
+  const timerlog = () => {
+    const now = Date.now();
+    console.log(formatDuration(now - prev));
+    prev = now;
+  };
+  server.watcher.on("all", (...args) => {
+    timerlog();
+    reloadOnChange(...args);
+  });
   return server;
 }
 function createFileReloader(server) {
image

I patched the code.

@1natsu172
Copy link
Contributor

@kazu-ux @pnd280 Thanks both. Hmm It is taking longer than thought. About 800ms max.... I found it helpful. I'll keep you posted on any progress!

@aklinker1
Copy link
Collaborator

Chokidor v4 just got dropped: https://github.com/paulmillr/chokidar/releases/tag/4.0.0

But we'll probably have to wait for Vite 6 before using it

@kazu-ux
Copy link
Author

kazu-ux commented Sep 24, 2024

I updated wxt to 0.19.10, the issue was resolved. Thank you!

@1natsu172
Copy link
Contributor

@kazu-ux Oh sorry, I forgot to tell you that it's released. Glad to hear it has been resolved! (cc: @pnd280).

Can adjust it by environment variable.

wxt/packages/wxt/src/types.ts

Lines 1280 to 1298 in efcb593

/**
* The milliseconds to debounce when a file is saved before reloading.
* The only way to set this option is to set the `WXT_WATCH_DEBOUNCE`
* environment variable, either globally (like in `.bashrc` file) or
* per-project (in `.env` file).
*
* For example:
* ```
* # ~/.zshrc
* export WXT_WATCH_DEBOUNCE=1000
* ```
* or
* ```
* # .env
* WXT_WATCH_DEBOUNCE=1000
* ```
* @default 800
*/
watchDebounce: number;

@aklinker1
Copy link
Collaborator

Gonna go ahead and close this as completed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants