Changed extension bundling from Vite to webpack

[tjcouch-sil]

• Changed from Vite to webpack
  • You can now import the same dependency twice in multiple webviews, and they will bundle independently
  • All webview imports into main now have ?inline at the end of the import
  • You can now put .tsx in your webview imports if you want (optional)
  • Changed html webview .ejs extension to .html since that feels more natural and we can do that now
  • Added support for react/jsx-runtime for newer React syntax
  • Images import as data urls (though this isn't supported in Paranext currently. Maybe it will be later)
  • Using SWC to compile TypeScript code
  • Stopped bundling evil extension because webpack was being greedy and absorbing the imports, ignoring thrown errors, and giving {} empty object for unresolved imports. Not that this breaks anything; just not as good testing
  • Added some support for importing CommonJS modules and what appear to be ES Modules because webpack was making the imports look not like CommonJS modules to the extension host in production. Still not exactly sure what was wrong there
• Changed temp-vite to temp-build
• Changed extension folder structure: source files lib -> src, build files dist, distributable zip files release
• Added documentation on special features of the template
• Added packaging steps to create extension zip in release directory
• Made @sillsdev/scripture an external dependency since many extensions will likely share it
• Added new Hello World React 2 WebView for testing bundling all dependencies into each WebView independently (the main reason to switch away from Vite)
  • Also added helloWorld.onHelloWorld network event to demonstrate using them. All the hello world webviews sync up their click count (though this would probably be easier with a data provider)
• Added --logLevel command-line-argument

Resolves #232

---

This change is 

[irahopkinson]

Sorry, only partial review before my day ended.

Reviewed 23 of 69 files at r1, all commit messages.
Reviewable status: 23 of 69 files reviewed, 1 unresolved discussion (waiting on @tjcouch-sil)

---

extensions/src/hello-someone/hello-someone.web-view.html line 199 at r1 (raw file):

      else setupWebView();

      //# sourceURL=hello-someone.web-view.js

Shouldn't this be .html now?

[lyonsil]

This PR is behind main by a few weeks, I think. It might be helpful to rebase/merge with main so that all the conflicts are sorted out sooner than later. For example, this PR updates @sillsdev/scripture from 1.2.0 to 1.3.0, but main is already on 1.4.0. That might be the only conflict, but I wonder how many others there might be.

Reviewed all commit messages.
Reviewable status: 23 of 69 files reviewed, 1 unresolved discussion (waiting on @tjcouch-sil)

[lyonsil]

Reviewed 18 of 69 files at r1.
Reviewable status: 23 of 69 files reviewed, 1 unresolved discussion (waiting on @tjcouch-sil)

[irahopkinson]

Wow that looks like a lot of configuring, thanks for all your hard work on this. I'll publish this as is then look at your recent changes next.

Reviewed 46 of 69 files at r1.
Reviewable status: 62 of 70 files reviewed, 5 unresolved discussions (waiting on @lyonsil and @tjcouch-sil)

---

extensions/webpack/web-view-resolve-webpack-plugin.ts line 113 at r1 (raw file):

  }
}

Thanks for all the coments in this file.

---

extensions/webpack/webpack.config.main.ts line 44 at r1 (raw file):

      globalObject: 'globalThis',
      library: {
        type: 'umd',

Err... why UMD library? AFAIK thats a really old format and not used much any more. As it is I'd generally advocate moving to ESM over CJS.

---

extensions/webpack/webpack.util.ts line 192 at r1 (raw file):

          library: {
            name: extension.dirName,
            type: 'umd',

Again output to UMD - I don't recommened this. AFAIK UMD supports switching between AMD and CJS but it doesn't (nor can it) support ESM. But I could be out of date.

---

src/extension-host/services/extension.service.ts line 180 at r1 (raw file):

  )) as AmbiguousExtensionModule;
  // Get the actual extension module based on what kind of module it is
  // __esModule is a property built into node. We're just using it here

Above I asked about the webpack output of UMD not being compatible with ESM. I guess this is how you are getting around that incomaptibility but I don't understand what is going on here.

Code quote:

__esModule

---

extensions/src/hello-world/hello-world.ts line 65 at r1 (raw file):

 * Simple web view provider that provides other React web views when papi requests them
 */
const reactWebView2Provider: IWebViewProvider & { webViewType: string } = {

This is not DRY - it's repeated 3 times. Either add webViewType: string as an optional property of IWebViewProvider or declare an inherited type in this file or back in src\shared\models\web-view-provider.model.ts.

This makes me wonder if it should be a required property of IWebViewProvider since it's needed to register the WebView provider. This is out of scope for this PR so this could be a separate issue.

Code quote:

IWebViewProvider & { webViewType: string }

[irahopkinson]

I pulled the branch and it is behaving well. There are a bunch of new MUI warnings in the browser console though. Also the fetching of JHN 11:35 from the USFM data provider hasn't worked yet on any of my runs (but it is fetching the other verses in the Hello World React tab so the provider is running). But startup and shutdown are much faster and cleaner so far - nice job.

Reviewed 7 of 7 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @tjcouch-sil)

---

jest.config.ts line 9 at r3 (raw file):

const {
  config: { compilerOptions },
} = typescript.parseConfigFileTextToJson('tsconfig.json', fs.readFileSync('tsconfig.json', 'utf8'));

That's a handy util to know about. Thanks. Was this needed because of the comments in tsconfig.json?

Code quote:

typescript.parseConfigFileTextToJson

---

extensions/src/hello-world/web-views/hello-world.web-view.html line 17 at r3 (raw file):

    <div><button id="hello-someone" type="button">Hello Someone</button></div>
    <div id="hello-someone-output"></div>
    <div id="root"></div>

We end up with a second "Hello World" button inside this div. I guess that is testing raw React right?

[lyonsil]

Reviewed 20 of 69 files at r1, 1 of 7 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @tjcouch-sil)

[tjcouch-sil]

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @irahopkinson)

---

jest.config.ts line 9 at r3 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

That's a handy util to know about. Thanks. Was this needed because of the comments in tsconfig.json?

Yep! This is TypeScript's special json parser that ignores comments, allows dangling commas, etc.

---

extensions/webpack/webpack.config.main.ts line 44 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Err... why UMD library? AFAIK thats a really old format and not used much any more. As it is I'd generally advocate moving to ESM over CJS.

AMD is very old. UMD is a special type that webpack allows that makes the module polymorphic to AMD, CJS, and global variable. It tries to import as it thinks the loader is expecting. I thought maybe this was causing the ambiguous module issues in extension.service.ts, but it didn't fix when I changed it to 'commonjs'. But then you got me thinking, and I dug deeper and found 'commonjs-static'! I think this is pretty much what vite was doing before, and it is doing the same thing in dev and production (the module imports with activate on it instead of inside default). Thanks for pushing me to look harder.

I think I tried using ESM when I was first getting into loading extension code (before I understood webpack very well at all), and it didn't work for some reason. I bet it has something to do with how ERB configured the environment. It could be nice to update everything and not have to worry about CJS anymore. But right now I think everything pretty much needs to be CJS. I'll make an issue for investigating it.

Actually it seems the topic of ES vs CJS is rather split...
https://gist.github.com/sindresorhus/a39789f98801d908bbc7ff3ecc99d99c
https://gist.github.com/joepie91/bca2fda868c1e8b2c2caf76af7dfcad3 (warning: some language)

---

extensions/webpack/webpack.util.ts line 192 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Again output to UMD - I don't recommened this. AFAIK UMD supports switching between AMD and CJS but it doesn't (nor can it) support ESM. But I could be out of date.

Indeed UMD does support only AMD, CJS, and global variable. I changed this. See comment above.

---

src/extension-host/services/extension.service.ts line 180 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Above I asked about the webpack output of UMD not being compatible with ESM. I guess this is how you are getting around that incomaptibility but I don't understand what is going on here.

I didn't understand at first either; webpack was doing weird things on umd. Now that I'm using commonjs-static, it seems to be consistent! But I left the code for compatibility because why not

---

extensions/src/hello-someone/hello-someone.web-view.html line 199 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Shouldn't this be .html now?

I debated what to call it, honestly. But I thought maybe js would be clearer since it's just the script tag, not the whole html file, that Chrome displays. If you made two script tags, you would have to name them separately. I dunno; it's really hard to say. What's expected? I'll change to html for now. Let me know your thoughts.

---

extensions/src/hello-world/hello-world.ts line 65 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

This is not DRY - it's repeated 3 times. Either add webViewType: string as an optional property of IWebViewProvider or declare an inherited type in this file or back in src\shared\models\web-view-provider.model.ts.

This makes me wonder if it should be a required property of IWebViewProvider since it's needed to register the WebView provider. This is out of scope for this PR so this could be a separate issue.

I just decided to include the webViewType in the provider itself on a whim here. It's not required to do it this way as the webViewType is passed into the registration function.

I debated making it required on the webView and just inferring it when you register the provider, but I think that would disallow you from using one twice for two different types (which I guess could come in handy in some situation...? I dunno; seems nice to be flexible about it). I'll make another type that has it in this file. I don't know if it should go on the papi, though... Thoughts?

---

extensions/src/hello-world/web-views/hello-world.web-view.html line 17 at r3 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

We end up with a second "Hello World" button inside this div. I guess that is testing raw React right?

Yep, there's an HTML and a React button in this web view that do the same thing.

[irahopkinson]

Reviewed 8 of 8 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @tjcouch-sil)

---

extensions/webpack/webpack.config.base.ts line 16 at r4 (raw file):

 * The module format of library we want webpack to use for externals and create for our extensions
 *
 * @see webpack.Configuration['externalsTye'] for info about external import format

nit: typo

Code quote:

externalsTye

[tjcouch-sil]

Regarding not getting the USFM data provider: I think maybe the dotnet data provider is taking longer to load than the extension host + quick-verse + 5 seconds (our request timeout), so it's timing out. If you restart the extension host after starting, it doesn't happen again. It also doesn't happen in production. I think maybe webpack is just taking more system resources and making the dotnet provider too slow to start or something, but that's just a theory honestly. I haven't dug in too much. I increased the timeout to 10 seconds when not packaged, and I plan to file an issue regarding seeing if we can collapse some of these webpack processes into one with multiple configs in an array. I'm not sure yet why they aren't already set up this way to be honest. I could alternatively go ahead and mess around with that now if you'd prefer since increasing the timeout isn't really a very good option. Let me know your thoughts! Thanks! :)

Regarding the MUI warning: I have literally no idea why that doesn't show up in main. That's a problem with ref-selector.component.tsx which hasn't been touched in 2 weeks. I made some fast adjustments, but I don't really even like what I did (put in a new unwieldy prop that seems very MUI-specific and not that helpful. Would be better to just refactor it well, but I didn't want to take the time to look into it right now). Will file an issue.

Reviewable status: complete! all files reviewed, all discussions resolved

---

extensions/webpack/webpack.config.base.ts line 16 at r4 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

nit: typo

Done. Thanks!

[irahopkinson]

Reviewed 13 of 13 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

[lyonsil]

Reviewed 22 of 69 files at r1, 2 of 7 files at r2, 7 of 8 files at r4, 13 of 13 files at r5, 2 of 2 files at r6, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @tjcouch-sil)

---

extensions/src/hello-world/web-views/hello-world-2.web-view.tsx line 12 at r4 (raw file):

} = papi;

globalThis.webViewComponent = function HelloWorld2() {

Is this a normal thing we're expecting webViews to do? It seems very fragile if there are multiple webViews trying to render at the same time.

---

extensions/webpack/webpack.config.base.ts line 21 at r6 (raw file):

// commonjs-static formats the code to export everything on module.exports.<export_name> so it works
// well in cjs or esm https://webpack.js.org/configuration/output/#type-commonjs-static
export const LIBRARY_TYPE: NonNullable<webpack.Configuration['externalsType']> = 'commonjs-static';

When I was digging into another issue recently, I read in several places the ESMs work much better with tree shaking than CJS. I think you have a ticket to investigate moving to ESM, but I just wanted to comment that doing everything in CJS is may be bloating our packages.

---

extensions/webpack/webpack.config.base.ts line 41 at r6 (raw file):

  // Modules that Paranext supplies to extensions https://webpack.js.org/configuration/externals/
  // All other dependencies must be bundled into the extension
  externals: [

Should the following components be in this list, too? That might be the cause of new MUI warnings.

@emotion/react
@emotion/styled
@mui/material

In a recent change of mine I added these to a peer dependencies list to resolve some MUI warnings.
c44a814

---

lib/papi-dts/papi.d.ts line 57 at r6 (raw file):

    var resourcesPath: string;
    /**
     * How much logging should be recorded. Defaults to 'info' if not packaged, 'error' if packaged

Very minor nit, but it looks like the source of this comment didn't use 1-line comments like the other variables preceding it.

---

src/shared/services/network.service.ts line 181 at r5 (raw file):

  // This approach is hacky but works well enough for now
  const expectedErrorMsg: string = `No handler was found to process the request of type ${requestType}`;
  const maxAttempts: number = globalThis.isPackaged ? 5 : 10;

Isn't this backwards? If it's packaged, isn't that when you said in your top-level PR comments that you wanted to increase the timeout period?

---

extensions/src/evil/evil.js line 15 at r4 (raw file):

    // This will be blocked and will suggest the papi.storage api.
    const fs = require('fs');
    logger.error(`Evil: Successfully imported fs! fs.readFileSync = ${fs.readFileSync}`);

When I'm looking at the console I find myself wondering sometimes if the "Evil" output is telling me something good or something bad. If you're tweaking these anyway it might be nice to put "GOOD" or "BAD" near the start of each message to make it clear. Sometimes I'm not looking at a console with color, and that's the only obvious difference between "info" and "error" I think.

[tjcouch-sil]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @lyonsil)

---

extensions/src/hello-world/web-views/hello-world-2.web-view.tsx line 12 at r4 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Is this a normal thing we're expecting webViews to do? It seems very fragile if there are multiple webViews trying to render at the same time.

Yes, this is how we have been getting the component for a while now. Each iframe has its own global context. Though it may get tricky if we want to share webviews without wrapping them in a new iframe. Maybe we ought to consider how we can improve this. The challenge is that this code is executed directly in a script on the frontend, not imported anywhere as a module. I made issue #323 to investigate revising this (will fill it in later)

---

extensions/webpack/webpack.config.base.ts line 21 at r6 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

When I was digging into another issue recently, I read in several places the ESMs work much better with tree shaking than CJS. I think you have a ticket to investigate moving to ESM, but I just wanted to comment that doing everything in CJS is may be bloating our packages.

Thanks for the info! #321

---

extensions/webpack/webpack.config.base.ts line 41 at r6 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Should the following components be in this list, too? That might be the cause of new MUI warnings.

@emotion/react
@emotion/styled
@mui/material

In a recent change of mine I added these to a peer dependencies list to resolve some MUI warnings.
c44a814

Unfortunately importing component libraries is not simple as component libraries usually have code that ties styles from each component into the document when the component is imported. If you import a component in the renderer main frame then pass it down to extensions' iframes, they will not have styles. We saw this issue with built-in papi react components for a while, which is one reason we made them their own independent library. I'm not sure about emotion stuff (it might pass down just fine! Probably worth some investigation), but it seems we need to bundle mui/material into papi-components for the reasons I just mentioned. I don't know that any of our extensions have tried using emotion yet (I do not use it personally). If you would like to file an issue to investigate providing these libraries instead of bundling them into extensions, you're welcome to do so.

---

lib/papi-dts/papi.d.ts line 57 at r6 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Very minor nit, but it looks like the source of this comment didn't use 1-line comments like the other variables preceding it.

Unfortunately a one-liner would be literally 101 characters :( so it's multi-line

aaaaagh 😂

---

src/shared/services/network.service.ts line 181 at r5 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Isn't this backwards? If it's packaged, isn't that when you said in your top-level PR comments that you wanted to increase the timeout period?

In development, everything takes so long to start up and such that I seemingly needed to increase the timeout. I figured it's best to leave everything quick in production. Hopefully we won't need this to be dependent on packaging after #313

---

extensions/src/evil/evil.js line 15 at r4 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

When I'm looking at the console I find myself wondering sometimes if the "Evil" output is telling me something good or something bad. If you're tweaking these anyway it might be nice to put "GOOD" or "BAD" near the start of each message to make it clear. Sometimes I'm not looking at a console with color, and that's the only obvious difference between "info" and "error" I think.

Done. Good call!

[lyonsil]

Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

---

extensions/webpack/webpack.config.base.ts line 41 at r6 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Unfortunately importing component libraries is not simple as component libraries usually have code that ties styles from each component into the document when the component is imported. If you import a component in the renderer main frame then pass it down to extensions' iframes, they will not have styles. We saw this issue with built-in papi react components for a while, which is one reason we made them their own independent library. I'm not sure about emotion stuff (it might pass down just fine! Probably worth some investigation), but it seems we need to bundle mui/material into papi-components for the reasons I just mentioned. I don't know that any of our extensions have tried using emotion yet (I do not use it personally). If you would like to file an issue to investigate providing these libraries instead of bundling them into extensions, you're welcome to do so.

Ok - I'm not primarily concerned with providing vs bundling per se. I am just thinking about eliminating warnings and errors coming from our dependencies. I haven't been running this new branch yet, but if there are new MUI warnings then we will need a ticket to eliminate them.

[lyonsil]

Reviewable status: complete! all files reviewed, all discussions resolved

[tjcouch-sil]

Reviewable status: complete! all files reviewed, all discussions resolved

---

extensions/webpack/webpack.config.base.ts line 41 at r6 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Ok - I'm not primarily concerned with providing vs bundling per se. I am just thinking about eliminating warnings and errors coming from our dependencies. I haven't been running this new branch yet, but if there are new MUI warnings then we will need a ticket to eliminate them.

I don't believe there are new MUI warnings. Jolie just reported the MUI issue Ira was seeing on my branch on a totally separate branch. I don't know why I didn't see that error when I tried main for a bit yesterday. I made a quick fix here anyway, but a better fix should be made long-term: #322