#113: Upgrade to Webpack 5 #421

fregante · 2021-06-03T16:52:14Z

Closes #113

This PR tries to be as minimal as possible. Build improvements will come in a future PR.

~~This appears to build and I'm pushing it here to double-check on the PR. I still need to test it and possibly refine it further. There are a lot of moving parts so it's taking a long while.~~

Related issues include:

However I did not encounter the HMR issue described in #113. Was that at build time or runtime?

Example: WARNING in DefinePlugin Conflicting values for 'process.env.NODE_ENV'

Raynos/global#21

Solved a warning, caused errors

fregante

Hallelujah CI passes

fregante · 2021-06-03T16:56:40Z

browsers/webpack/webpack.base.js

-  process: true,
-  Buffer: true,
-  console: true,
-  fs: "empty",


Webpack 5 no longer includes polyfills by default, so I ended up using https://github.com/Richienb/node-polyfill-webpack-plugin instead of installing 7 dependencies separately and configuring them here.

fregante · 2021-06-03T18:40:32Z

browsers/chrome/webpack/webpack.prod.js

-        filename: "css/[name].css",
-        chunkFilename: "css/[id].css",
-        ignoreOrder: false, // Enable to remove warnings about conflicting order
-      }),


This should already be inherited from webpack.base.js

fregante · 2021-06-03T18:44:31Z

scripts/webpack.scripts.js

    },
    extensions: [".ts", ".tsx", ".jsx", ".js"],
+    fallback: {
+      chokidar: false,


Some packages like handlebar and nunchucks do dependency checks via try {require()}, which cause webpack to bundle the dependency.

It seems that many of these dependencies are not required and should not be part of the browser anyway. As part of #411 I can look into the specific modules and find web-only entry points for each, if they exist.

Could you clarify what you mean by "and find web-only entry points for each" means? Not familiar with it

https://github.com/mozilla/nunjucks/blob/fd500902d7c88672470c87170796de52fc0f791a/nunjucks/src/node-loaders.js#L34-L38

The filename and option suggest that this file, which is required and bundled into PixieBrix, is actually meant to be run on Node. Some packages offer browser-only bundles (browser field in package.json) without all of this Node-specific cruft — I'll look for them.

Sub-dependencies like this one were resolvable at some point because some other package downloaded them, but I suppose that these changes resurfaced this (unnecessary) dependency mismatch and caused the build to fail.

I think I understand now -- e.g., you'd find an entry point to nunjucks that doesn't reference the node-loaders module. That would prevent webpack from looking for chokidar in the first place, so we wouldn't have to explicitly tell webpack not to resolve it. Is my understanding correct?

For the sake of expediency, I'd also be ok marking things as false to get the build to pass after we do a quick check like you did to make sure they're not going to cause a problem

That’s correct. The current code already builds and should match more or less what existed before, so dependency-wise it shouldn’t need further changes in this PR (I’d rather merge the upgrade first, and only then do optimizations).

I’ll run the extension today and see what’s missing.

fregante · 2021-06-03T19:59:01Z

The extension runs, but please do a quick run through to ensure it's not broken in some obvious ways.

twschiller

Getting some errors when running the extension on pages. Looks like one of these is related to the path changing for the css files?:

Denying load of chrome-extension://mpjjildhmpddojocokjkgmlkkkfjnepo/icons.css. Resources must be listed in the web_accessible_resources manifest key in order to be loaded by pages outside the extension.

In general, a good way to test big changes is by just running through the https://docs.pixiebrix.com/quick-start-guide quickly

twschiller · 2021-06-03T20:06:11Z

browsers/webpack/webpack.base.js

  ],
  module: {
    rules: [
      // https://github.com/webpack/webpack/issues/3017#issuecomment-285954512
      // prevent lodash from overriding window._
      {
        exclude: /(notifyjs-browser|vendors\/notify)/,
-        parser: { amd: false },


@fregante What was the reason for removing this? I had originally added this since having lodash overwrite window._ breaks sites running older versions of lodash

Sorry I blindly removed this line since it was deprecated:

Module not found: ValidationError: Invalid parser object. Json Modules Plugin has been initialized using a parser object that does not match the API schema. - parser has an unknown property 'amd'. These properties are valid: object { parse? }

I pushed a tentative solution for this. It compiles but I haven't run it.

Thanks, I can help show you how to test it. I think you just find a page where the PixieBrix contentScript is loaded and see if it set window._ or not.

To do that, you could just click the PixieBrix extension icon in the Chrome toolbar on any page to trigger the browser action, which will inject the contentScript and the side bar

I clicked the button

The sidebar appeared on the page

window._ is undefined in the top frame in the console

window._ is undefined in content script extension in the console

I also verified that without amd: false, window._ is a function in both context; This means amd: false fixes this Lodash issue. 🎉

However I think that this is breaking notify.js, so I will probably just drop the UMD from its bundle and adjust it to work correctly. The file is dated 2015 anyway.

twschiller · 2021-06-03T20:09:11Z

scripts/webpack.scripts.js

    },
    extensions: [".ts", ".tsx", ".jsx", ".js"],
+    fallback: {
+      chokidar: false,


Could you clarify what you mean by "and find web-only entry points for each" means? Not familiar with it

fregante · 2021-06-04T02:44:10Z

Something that confused me are the CSS files actually. It appears that most CSS files appear both at the entry point and inside the CSS folder.

Also there’s a CopyPlugin configuration in the prod files that copied everything from src, unchanged. What was the reason for that?

twschiller · 2021-06-04T03:24:00Z

Also there’s a CopyPlugin configuration in the prod files that copied everything from src, unchanged. What was the reason for that?

From browsers/chrome/webpack/webpack.prod.js:

{
   from: path.resolve(chromeRoot, "src"),
},
{
   from: path.resolve(chromeRoot, "..", "src"),
},

The first one ends up copying over the browsers/chrome/background.html file which differs by browser because the Google distribution has the gapi script tag, but Firefox does not

The second one copies over browsers/src files which has the html files and also the CSS for some of those files, which for whatever historic reasons I wrote directly in there vs. referencing it from an entry point so that webpack would build it. So short answer: no great reason for the CSS files that I can remember

fregante · 2021-06-04T19:38:57Z

Everything seems to be fine now. I don't see any errors other than #428, which also appears on main

twschiller · 2021-06-05T19:22:27Z

Testing the PR now

However I did not encounter the HMR issue described in #113. Was that at build time or runtime?

Will see if it's actually an issue. On package install, I get a peer dependency warning:

npm WARN webpack-extension-reloader@1.1.4 requires a peer of webpack@^4 but none is installed. You must install peer dependencies yourself.

fregante · 2021-06-06T03:31:51Z

Yeah that dependency was meant to be removed as discussed in #113 (comment), but I left it in so I can properly document what will replace it.

fregante added 2 commits June 4, 2021 00:07

Upgrade to webpack 5

819165c

Drop unused webpack dependencies

bb5871e

fregante force-pushed the feature/webpack-5 branch from 48dc67d to bb5871e Compare June 3, 2021 17:11

fregante added 5 commits June 4, 2021 01:36

Fix process.env conflicts

2a9a22c

Example: WARNING in DefinePlugin Conflicting values for 'process.env.NODE_ENV'

Fix global conflict with ESM

d58b7ff

Raynos/global#21

Fix remaining runtime error

3afd3c9

Undo unnecessary DefinePlugin changes

253e863

Solved a warning, caused errors

GHA fail: Attempt increasing mem size

d63b74d

fregante commented Jun 3, 2021

View reviewed changes

fregante marked this pull request as ready for review June 3, 2021 19:37

fregante mentioned this pull request Jun 3, 2021

Merge webpack configs #422

Closed

fregante changed the title ~~#113: Update to Webpack 5~~ #113: Upgrade to Webpack 5 Jun 3, 2021

twschiller requested changes Jun 3, 2021

View reviewed changes

Tentative fix for amd: false option for lodash

3162af4

fregante added 2 commits June 4, 2021 18:20

Fix CSS loading issue

1391468

Drop UMD from notify.js to fix bundling

705c038

fregante requested a review from twschiller June 4, 2021 19:38

twschiller approved these changes Jun 5, 2021

View reviewed changes

twschiller merged commit 89231cd into main Jun 5, 2021

twschiller deleted the feature/webpack-5 branch June 5, 2021 19:26

fregante mentioned this pull request Jun 6, 2021

#422: Simplify webpack configuration and merge it #435

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#113: Upgrade to Webpack 5 #421

#113: Upgrade to Webpack 5 #421

fregante commented Jun 3, 2021 •

edited

Loading

fregante left a comment

fregante Jun 3, 2021

fregante Jun 3, 2021

fregante Jun 3, 2021

twschiller Jun 3, 2021

fregante Jun 3, 2021 •

edited

Loading

twschiller Jun 4, 2021

fregante Jun 4, 2021

fregante commented Jun 3, 2021

twschiller left a comment

twschiller Jun 3, 2021

fregante Jun 3, 2021

twschiller Jun 4, 2021

fregante Jun 4, 2021 •

edited

Loading

twschiller Jun 3, 2021

fregante commented Jun 4, 2021

twschiller commented Jun 4, 2021 •

edited

Loading

fregante commented Jun 4, 2021

twschiller commented Jun 5, 2021 •

edited

Loading

fregante commented Jun 6, 2021

#113: Upgrade to Webpack 5 #421

#113: Upgrade to Webpack 5 #421

Conversation

fregante commented Jun 3, 2021 • edited Loading

fregante left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fregante Jun 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fregante commented Jun 3, 2021

twschiller left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fregante Jun 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fregante commented Jun 4, 2021

twschiller commented Jun 4, 2021 • edited Loading

fregante commented Jun 4, 2021

twschiller commented Jun 5, 2021 • edited Loading

fregante commented Jun 6, 2021

fregante commented Jun 3, 2021 •

edited

Loading

fregante Jun 3, 2021 •

edited

Loading

fregante Jun 4, 2021 •

edited

Loading

twschiller commented Jun 4, 2021 •

edited

Loading

twschiller commented Jun 5, 2021 •

edited

Loading