Use more smaller files and imports #11

wheelercj · 2024-05-09T01:06:51Z

The background and content scripts should be split into multiple files to improve organization and to reuse some duplicate code.

In particular, some of the functions are defined multiple times for each browser extension, such as getSetting which is defined 5 times total.

The text was updated successfully, but these errors were encountered:

wheelercj · 2024-05-30T23:28:25Z

Here are some notes about importing in background.js in Firefox. Mozilla's docs should explain how to get this working, but they don't.

For context, Stardown uses Firefox manifest v2 because of this bug: 1851083 - Manifest V3 extensions with activeTab/tabs permission require user interaction not previously required, showing blue dot on desktop and no indicator on Android.

I created common.js and defined this function:

export async function getSetting(name, default_) {
    ...
}

In background.js, I added import { getSetting } from './common.js'; to the top and removed the getSetting definition.

Loading the extension gives the error Uncaught SyntaxError: import declarations may only appear at top level of a module.

I made background.js a module by adding "type": "module" to manifest.json:

"background": {
    "scripts": [
        "background.js",
        "common.js"
    ],
    "type": "module"
},

This changes the error message to Tried to get undefined setting "doubleClickInterval", which is printed when getSetting's await browser.storage.sync.get call returns undefined. This happens because currently, when Stardown loads, it immediately calls getSetting('doubleClickInterval', 500).then(value => doubleClickInterval = value).

Wrapping the getSetting call with browser.runtime.onInstalled.addListener(() => { ... }); doesn't fix the error or change the error message. Neither does using the code below in place of import { getSetting } from './common.js'; and the top-level getSetting call.

let getSetting;
import('./common.js').then((exports) => {
    getSetting = exports['getSetting'];
    getSetting('doubleClickInterval', 500).then(value => doubleClickInterval = value);
});

Neither does replacing "type": "module" in manifest.json with "page": "background.html" and using this HTML in background.html:

<!DOCTYPE html>
<html>

  <body>
    <script type="module" src='./background.js'></script>
  </body>

</html>

Neither does using both "type": "module" and "page": "background.html" in manifest.json.

I put "type": "module" back in manifest.json and removed "page": "background.html".

When I comment out getSetting('doubleClickInterval', 500).then(value => doubleClickInterval = value);, the extension loads with no errors. However, clicking the extension's icon gives the error Error: No response received from the content script.

Uncommenting the getSetting call, removing import { getSetting } from './common.js';, removing export, and removing "type": "module" also gives the error Error: No response received from the content script.

Using this in manifest.json:

"background": {
    "page": "background.html"
},

and this background.html:

<!DOCTYPE html>
<html>

  <body>
    <script type="module" src='./common.js'></script>
    <script type="module" src='./background.js'></script>
  </body>

</html>

with the import and export keywords gives the error Tried to get undefined setting "doubleClickInterval". Without them gives Uncaught ReferenceError: getSetting is not defined.

Removing both instances of type="module" from the HTML above (and still not using import or export allows the extension to mostly work in Firefox! However, clicking the extension's icon now displays some Mojibake instead of a check, and Chromium probably requires unconditional use of import and export.

Maybe Firefox manifest v2 doesn't allow the use of import and export? Their documentation is missing something. We should probably just use a bundler.

One of the sources I looked at for ideas: [WebExtension] Import a module in a script

wheelercj · 2024-06-08T04:24:34Z

About the problem with Chrome giving us the error message Uncaught SyntaxError: Cannot use import statement outside a module from content.js:17, I tried enabling Chrome's verbose logging with chrome.exe --enable-logging --v=1 (looks like the Linux equivalent might be google-chrome --enable-logging --v=1), but the logs don't seem to have anything helpful. I'm putting the relevant parts of the logs below anyways. My local test copy of Stardown has the Chrome extension ID ckghjmcmjkaefnkmobodopfihgkeehno. The .cc files mentioned don't appear to be in the https://github.com/chromium/chromium repo. I was surprised to see Chrome search for manifest files in folders Stardown/chrome and Stardown/chromium since Stardown hasn't had those folders for a while.

Chrome version 125.0.6422.142

...

[6764:6656:0607/204034.346:VERBOSE1:script_context.cc(149)] Created context:
extension id: ckghjmcmjkaefnkmobodopfihgkeehno
frame: 0000000000000000
URL:
context_type: BLESSED_EXTENSION
effective extension id: ckghjmcmjkaefnkmobodopfihgkeehno
effective context type: BLESSED_EXTENSION
[6764:6656:0607/204034.347:VERBOSE1:script_context.cc(149)] Created context:
extension id: (none)
frame: 0000000000000000
URL:
context_type: UNSPECIFIED
effective extension id: (none)
effective context type: UNSPECIFIED

...

[8796:6160:0607/204040.099:VERBOSE1:script_context.cc(149)] Created context:
extension id: ckghjmcmjkaefnkmobodopfihgkeehno
frame: 00001FF10014E540
URL:
context_type: CONTENT_SCRIPT
effective extension id: ckghjmcmjkaefnkmobodopfihgkeehno
effective context type: CONTENT_SCRIPT
[8796:6160:0607/204040.099:VERBOSE1:script_context.cc(149)] Created context:
extension id: (none)
frame: 0000000000000000
URL:
context_type: UNSPECIFIED
effective extension id: (none)
effective context type: UNSPECIFIED
[8796:6160:0607/204040.100:VERBOSE1:dispatcher.cc(419)] Num tracked contexts: 3
[8476:22148:0607/204040.119:INFO:CONSOLE(17)] "Uncaught SyntaxError: Cannot use import statement outside a module", source: chrome-extension://ckghjmcmjkaefnkmobodopfihgkeehno/content.js (17)

...

[8476:22148:0607/204025.928:WARNING:load_error_reporter.cc(72)] Extension error: Failed to load extension from: C:\Users\chris\Documents\programming\Stardown\chrome. Manifest file is missing or unreadable
[8476:22148:0607/204025.930:WARNING:load_error_reporter.cc(72)] Extension error: Failed to load extension from: C:\Users\chris\Documents\programming\Stardown\chromium. Manifest file is missing or unreadable

...

[6764:6656:0607/204434.453:VERBOSE1:script_context.cc(162)] Destroyed context for extension
extension id: ckghjmcmjkaefnkmobodopfihgkeehno
effective extension id: ckghjmcmjkaefnkmobodopfihgkeehno
[6764:6656:0607/204434.453:VERBOSE1:script_context.cc(162)] Destroyed context for extension
extension id:
effective extension id:

wheelercj · 2024-06-08T05:04:25Z

Continuing with trying to fix Chrome's Uncaught SyntaxError: Cannot use import statement outside a module, I tried temporarily moving all of Stardown's content code into content.js so that it doesn't use import. It worked fine, as expected. Then I created a file called browser.js with export const browser = chrome; and added one import to content.js: import { browser } from './browser.js';. It still gave the same error message as before both with and without "type": "module" in manifest.json in the content_scripts object. When I added "browser.js" to the list of content scripts in manifest.json, the same error message appeared as well as Uncaught SyntaxError: Unexpected token 'export' from browser.js, again both with and without "type": "module".

I repeated all of these steps again with the urlEncode function instead of the browser variable and got all the same results.

Not even the most simple cases of importing and exporting are working for Chrome content scripts. We may need to use a bundler for Chrome as well.

wheelercj · 2024-06-08T06:08:04Z

From javascript - How to import ES6 modules in content script for Chrome Extension - Stack Overflow, many people don't expect to be able to use or need import and export statements in content scripts. Apparently, content scripts are all loaded together in the order listed in the manifest as if they're one big file. However, some of Stardown's code used in the content is also used in the background, so it's not an easy choice to just remove the imports and exports. The linked discussion also talks about a function named import. I tried it and it works for the simple cases. I'll do a more thorough test with it soon. I wonder if it can work for the background scripts, and for Firefox. One obvious downside is that since it uses browser, that variable will need to be defined in almost every file instead of being defined in one and imported into the others.

wheelercj · 2024-06-08T21:11:49Z

Here is Stardown's current file structure:

flowchart TD
    menu.js --> chromiumConfig.js
    menu.js --> firefoxConfig.js
    common.js --> md.js

    chromiumConfig.js --> content.js
    firefoxConfig.js --> content.js
    common.js --> content.js
    md.js --> content.js
    createTextFragmentArg.js --> content.js
    
    chromiumConfig.js --> background.js
    firefoxConfig.js --> background.js
    common.js --> background.js
    md.js --> background.js

    common.js --> options.js

I don't see a way to change this so that it has no files that both import and export, unless we go back to using something like if (browserName === 'chromium') in many places and/or having repeated code.

@chizuo Changing Stardown to not use both import and export in any content files didn't fix the error we've been getting (more details in an earlier comment here), and to avoid (or at least minimize) repeated code and if/else statements determining which browser the code is running in, Stardown requires having some files that both import and export.

Using the import function instead of the keyword and defining browser in many places works in simple cases. Here's a better way to use the import function than what I tried before:

let menu;
(async () => {
    menu = await import(browser.runtime.getURL('./menu.js'));
})();

However, our use case is not simple enough for the import function; it gives the error message Uncaught (in promise) TypeError: import() is disallowed on ServiceWorkerGlobalScope by the HTML specification. See https://github.com/w3c/ServiceWorker/issues/1356. This error sounds like it could be fixed by using the import function for the content and the import keyword for the background, but as you can see in the diagram, the config files and md.js need to be used by both the content and the background, and they all need to import unless we have repeated code. We can't conditionally use one import or the other because An import declaration can only be used at the top level of a module.

It looks like we might have no choice but to use a bundler for Chrome.

Closes #11.

wheelercj self-assigned this May 9, 2024

wheelercj changed the title ~~Reorganize code~~ Deduplicate code May 21, 2024

wheelercj assigned chizuo May 29, 2024

wheelercj changed the title ~~Deduplicate code~~ Use more smaller files and imports May 30, 2024

wheelercj added a commit that referenced this issue Jun 11, 2024

Use a bundler for Chromium

38f2281

Closes #11.

wheelercj mentioned this issue Jun 11, 2024

Restructure #51

Merged

wheelercj closed this as completed in #51 Jun 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use more smaller files and imports #11

Use more smaller files and imports #11

wheelercj commented May 9, 2024 •

edited

Loading

wheelercj commented May 30, 2024 •

edited

Loading

wheelercj commented Jun 8, 2024 •

edited

Loading

wheelercj commented Jun 8, 2024

wheelercj commented Jun 8, 2024 •

edited

Loading

wheelercj commented Jun 8, 2024

Use more smaller files and imports #11

Use more smaller files and imports #11

Comments

wheelercj commented May 9, 2024 • edited Loading

wheelercj commented May 30, 2024 • edited Loading

wheelercj commented Jun 8, 2024 • edited Loading

wheelercj commented Jun 8, 2024

wheelercj commented Jun 8, 2024 • edited Loading

wheelercj commented Jun 8, 2024

wheelercj commented May 9, 2024 •

edited

Loading

wheelercj commented May 30, 2024 •

edited

Loading

wheelercj commented Jun 8, 2024 •

edited

Loading

wheelercj commented Jun 8, 2024 •

edited

Loading