feat(plugin-webpack): support standalone preload entry points

[erickzhao]

• I have read the contribution documentation for this project.
• I agree to follow the code of conduct that this project follows, as appropriate.
• The changes have sufficient test coverage (if applicable).
• The test suite passes successfully on my local machine (if applicable).

Summary

Closes #2859

A common pattern for Electron applications is to load a remote website into a BrowserWindow and augmenting it with a preload script without having a local index.html renderer process.

However, the webpack plugin does not support having a preload entry without an attached index.html file. This is an attempt at changing that.

API

Entry points can now be one of three types:

• BrowserWindow instances that load a local HTML file.
  • mandatory: html, js
  • optional: preload
• Renderer process JavaScript that doesn't need HTML (e.g. Web Workers).
  • mandatory: js
• A preload script that is meant for use with BrowserWindow instances that load remote content.
  • mandatory: preload

These are inferred via which fields are present or not in the config.

{
    "plugins": [
        [
          "./src/plugin-webpack/dist/WebpackPlugin.js",
          {
            "mainConfig": "./webpack.main.config.js",
            "renderer": {
              "config": "./webpack.renderer.config.js",
              "entryPoints": [
                {
                  "name": "erick",
                  "preload": {
                    "js": "./src/preload.ts"
                  }
                }
              ]
            }
          }
        ]
    ]
}

Demo

See https://github.com/erickzhao/ericktron-forge/tree/templated

declare const ERICK_PRELOAD_WEBPACK_ENTRY: string;

//...

  // Create the browser window.
  const mainWindow = new BrowserWindow({
    height: 600,
    width: 800,
    webPreferences: {
      preload: ERICK_PRELOAD_WEBPACK_ENTRY,
    },
  });

Questions

• Are the names good? Suggestions welcome!

• Do we even need the type property? Technically, all 3 current configurations can be inferred by the presence of the html/js/preload properties, so we can get away with not adding any new properties to the config. However, being explicit makes the config more readable and makes TypeScript support better.

[codecov]

Codecov Report

Merging #2950 (67242b5) into main (1cd68e3) will increase coverage by 2.14%.
The diff coverage is 75.67%.

@@            Coverage Diff             @@
##             main    #2950      +/-   ##
==========================================
+ Coverage   71.33%   73.47%   +2.14%     
==========================================
  Files          79       68      -11     
  Lines        2414     2202     -212     
  Branches      452      440      -12     
==========================================
- Hits         1722     1618     -104     
+ Misses        469      374      -95     
+ Partials      223      210      -13     

Impacted Files	Coverage Δ
packages/plugin/webpack/src/WebpackPlugin.ts	37.93% <0.00%> (-0.07%)	⬇️
packages/plugin/webpack/src/WebpackConfig.ts	94.62% <84.61%> (-4.10%)	⬇️
...kages/plugin/webpack/src/util/rendererTypeUtils.ts	100.00% <100.00%> (ø)
packages/maker/base/src/Maker.ts	69.69% <0.00%> (ø)
packages/api/core/src/api/make.ts	79.00% <0.00%> (ø)
packages/api/core/src/api/start.ts	64.04% <0.00%> (ø)
packages/maker/dmg/src/MakerDMG.ts	95.00% <0.00%> (ø)
packages/maker/wix/src/MakerWix.ts	81.81% <0.00%> (ø)
packages/maker/zip/src/MakerZIP.ts	100.00% <0.00%> (ø)
packages/plugin/base/src/Plugin.ts	57.14% <0.00%> (ø)
... and 36 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1cd68e3...67242b5. Read the comment docs.

[erickzhao]

The code works and I've fixed existing tests. Before writing new tests for this feature, I'd love some API feedback.

[malept]

So we're technically still in beta. We could just require everyone to add type. The more complaint-averse solution is to emit a warning that type will be required in the next major version.

[georgexu99]

LGTM, however I'm with @malept with changing type to be a required field. Right now the code comment indicatingWebpackPluginEntryPoint.type's default value is tightly coupled with code behaviour in WebpackConfig.ts, and it isn't immediately obvious that if we ever change the code behaviour we have to change the comment indicating that the default value of type is local-window. Making type default could be the solution here (non blocking though)

[erickzhao]

I think a deprecation warning for Forge 7 would be a good solution.

[MarshallOfSound]

I'm not clear on why we need a type field instead of just making certain property combinations possible / allowed.

E.g. Right now the js key is required and the html and preload keys are optional

If we instead document the type like so, we could simply infer what you're calling the "type" based on the present keys and error out if an invalid combination of keys is provided.

type Entry = {
  js: string;
} | {
  html: string;
  js: string;
} | {
  html: string;
  js: string;
  preload: WebpackPreloadEntryPoint;
} | {
  preload: WebpackPreloadEntryPoint;
}

I'm not sure if that's a better API but I don't like this type parameter floating around, it feels kinda boxy for something that doesn't have to be.

[VerteDinde]

API LGTM!

Is there anywhere in the docs where we can/need to update the examples for the webpack plugin (e.g.: local window, no window, preload only)? It might be nice to show those options for clarity.

[VerteDinde]

(Not holding it as a merge blocker, but it looks like our friend the linux-x64 fixture binary also got committed)

[erickzhao]

We should update the docs in accordance yep. Technically since this doesn't have any breaking changes we can just add it in later.

outdated