webextension-toolbox / webpack-webextension-plugin

Webpack plugin that compiles WebExtension manifest.json files and adds smart auto reload
https://www.npmjs.com/package/@webextension-toolbox/webpack-webextension-plugin
Other
83 stars 21 forks source link

Repeated file changes doesn't properly compile manifest #628

Open vwheezy opened 6 months ago

vwheezy commented 6 months ago

Using a mostly standard setup, the first dev compile outputs the correct manifest.json but every reload after uses the non-compiled version.

E.g. first compilation outputs:

manifest.json ```json { "name": "__MSG_appName__", "description": "__MSG_appDescription__", "version": "1.0.0", "short_name": "__MSG_appShortName__", "manifest_version": 3, "default_locale": "en", "background": { "scripts": [ "serviceWorker.js", "webextension-toolbox/background_page.js" ] }, "content_scripts": [ { "matches": [ "" ], "js": [ "contentScript.js" ] } ], "permissions": [ "storage", "scripting" ], "options_ui": { "page": "pages/options.html", "open_in_tab": true }, "action": { "default_title": "__MSG_browserActionTitle__", "default_popup": "pages/popup.html", "default_icon": { "16": "images/icon-16x.png", "32": "images/icon-32x.png", "48": "images/icon-48x.png", "128": "images/icon-128x.png" } }, "icons": { "16": "images/icon-16x.png", "32": "images/images-32x.png", "48": "images/icon-48x.png", "128": "images/icon-128x.png" }, "browser_specific_settings": { "gecko": { "id": "addon@example.com" } } } ```

Second compilation:

manifest.json ```json { "name": "__MSG_appName__", "short_name": "__MSG_appShortName__", "description": "__MSG_appDescription__", "manifest_version": 3, "default_locale": "en", "background": { "scripts": ["serviceWorker.js"] }, "content_scripts": [ { "matches": [""], "js": ["contentScript.js"] } ], "permissions": ["storage", "scripting"], "options_ui": { "page": "pages/options.html", "open_in_tab": true }, "action": { "default_title": "__MSG_browserActionTitle__", "default_popup": "pages/popup.html", "default_icon": { "16": "images/icon-16x.png", "32": "images/icon-32x.png", "48": "images/icon-48x.png", "128": "images/icon-128x.png" } }, "icons": { "16": "images/icon-16x.png", "32": "images/images-32x.png", "48": "images/icon-48x.png", "128": "images/icon-128x.png" }, "__firefox|safari__browser_specific_settings": { "gecko": { "id": "addon@example.com" } } } ```

First compile output shows:

asset manifest.json 1.08 KiB [emitted]

Second compile output shows:

asset manifest.json 997 bytes [emitted] [from: manifest.json] [copied]

I've outputted the changedFiles variables and it's everything in the source tree excluding dist, but including manifest.json for some reason.

Tried using --copy-ignore but couldn't figure out how to inject "packages" with globs.

If this is a code issue, I could help submit a PR. If this is a setup issue, any help would be greatly appreciated!

vwheezy commented 6 months ago

I tried building the codebase, but I was getting a lot of errors without making any modifications. Some instructions would be appreciated and I could submit a CONTRIBUTING.md to help the next developers.

tm1000 commented 6 months ago

@vwheezy have you looked at https://github.com/webextension-toolbox/webextension-toolbox which uses this plugin?

vwheezy commented 6 months ago

@tm1000 Yes, that's what I'm using currently. I thought the issue was with that codebase, but I'm fairly certain I tracked the issue down to here.

My --copy-ignore comment was actually in reference to webextension-toolbox (I was very tired writing that issue 😅)

vwheezy commented 6 months ago

How do you build the project? I have a good amount of time to try and fix this, plus I'll write some documentation for the future.

I'm new to developing npm packages, but trying to rebuild the codebase using npm run build-plugin & npm run build-client from inside node_modules does not work. However, it does when I clone the repo separately

tm1000 commented 6 months ago

@vwheezy I believe I fixed those issues so please git pull and run npm upgrade.

If you still have errors then please post them here. Thanks

vwheezy commented 6 months ago

@tm1000 Ah so frustrating, I had gotten it working (without knowing how) and then I changed something and it's not working again (without knowing how) 🙂.

I don't think your changes fixed it. I tried it using both npm link and adding the path of my local copy of the repo in package.json.

Something I noticed: before I don't think I had the webextension-toolbox directory being emitted to dist. It's being emitted now, but the manifest is still incorrect. Additionally, when it first started working dist wasn't separating into different vendor folders. Not sure what that was about.

Thanks so much for the help. Let me know if there's anything I can do, I'm just banging my head against a wall trying to recreate the conditions where it was working.

vwheezy commented 6 months ago

Completely forgot to mention this a long time ago: any change in manifest.json correctly rebuilds manifest.json, any other modification noticed by webpack, including changes in messages.json and changes in dependencies of manifest.json, does not recompile the manifest correctly.

vwheezy commented 6 months ago

I got it to work by forcing the manifestChanged boolean to be true, but I'm sure that's not intended behavior.

I changed detectManifestModification to this:

  detectManifestModification(compiler: Compiler) {
    if (compiler.modifiedFiles && compiler.options.context) {
      console.log("!!! compiler modified files !!!", compiler.modifiedFiles);
      const manifestFile = path.join(
        compiler.options.context,
        this.manifestNameDefault
      );
      // this.manifestChanged = compiler.modifiedFiles.has(manifestFile);
      this.manifestChanged = true;
    }
  }
tm1000 commented 6 months ago

What you could do is see the value of manifestFile and compare it to the list in compiler.modifiedFiles I'm thinking they are not matching after the first go-around

vwheezy commented 6 months ago

I'm not sure if this function is the issue. It correctly detects if manifestChanged. In my application, I'm not modifying the manifest, but when I do it forces a rebuild of the manifest.

I see in addManifest that it only rebuilds the manifest if it was changed, which also makes sense.

In keepFiles, I'm not sure how the CleanPlugin is used/called in the cycle of the plugin. I printed out the asset that would be matched against the function and only background_page.js shows up, not manifest.json. Is this the issue?

Also, changedFiles in reloadExtensions shows all of the files on every compile except for the first one, where it only shows the package output (xpi, zip, etc), background_page.js, and manifest.json. AFAICU, reloadExtensions doesn't actually affect the compilation of the files though, it just reloads the client side.

tm1000 commented 6 months ago

Actually this appears to be an issue in https://github.com/webextension-toolbox/webextension-toolbox where the copy plugin is copying manifest.json back over. The fix is listed below for webextension-toolbox, havent applied it yet because I am cleaning up this plugin to make more sense.

diff --git a/src/common/webpack.ts b/src/common/webpack.ts
index c5ed136..ef569c8 100644
--- a/src/common/webpack.ts
+++ b/src/common/webpack.ts
@@ -137,8 +137,10 @@ export default async function webpackConfig({
     path: resolvedTarget,
     filename: "[name].js",
     chunkFilename: "[id].chunk.js",
+    // https://github.com/webpack/webpack-dev-middleware/issues/861
     clean: true,
   };
+
   /** *************************** */
   /*    WEBPACK.OPTIMIZATION    */
   /** *************************** */
@@ -241,6 +243,9 @@ export default async function webpackConfig({
     ignore: compileIgnoredArray,
   });

+  // Add manifest to compiled files list
+  compiledFiles.push(resolve(src, "manifest.json"));
+
   // Copy non compiled files
   config.plugins.push(
     new CopyPlugin({
@@ -251,7 +256,7 @@ export default async function webpackConfig({
           context: resolve(src),
           from: resolve(src, "**/*").replace(/\\/g, "/"),
           globOptions: {
-            ignore: [...copyIgnore, "manifest.json", ...compiledFiles],
+            ignore: [...copyIgnore, ...compiledFiles],
           },
           to: resolvedTarget,
         },
vwheezy commented 6 months ago

Briefly tested and it seems to work, thanks so much for the work!

I'll submit an issue and PR to the project and link it to this issue.

Not sure if this is the right place to ask this, but I've familiarized myself with this codebase and wanted to help contribute to this project more. Any issues, features, or documentation you need done?