nextstrain / auspice

Web app for visualizing pathogen evolution
https://docs.nextstrain.org/projects/auspice/
GNU Affero General Public License v3.0
291 stars 162 forks source link

Webpack asset/resource module incorrectly handling client-side extensions using `require` (`ReferenceError: require is not defined`) #1617

Open jameshadfield opened 1 year ago

jameshadfield commented 1 year ago

Current Behavior

The nextstrain.org-customised version of auspice is broken as a png resource there is not correctly transformed by auspice's webpack configuration.

How to reproduce

  1. Open a shell prompt in the Auspice root directory.

  2. Run:

    npm ci --ignore-scripts
    
    # Download nextstrain.org with customisations to reproduce this issue
    git clone https://github.com/nextstrain/nextstrain.org --branch victorlin/repro-auspice-issue-1617 --single-branch --depth 1
    pushd nextstrain.org
    npm ci
    popd
    
    # Build webpack bundle using Auspice customisations
    rm -rf dist/
    node auspice.js build --verbose --extend nextstrain.org/auspice-client/customisations/config.json
    
    # Fetch example data
    mkdir -p data
    curl http://data.nextstrain.org/zika.json --compressed -o data/zika.json
    
    # Start Auspice server
    auspice view --verbose
  3. Load http://localhost:4000/zika -- the app crashes immediately with a console error Uncaught (in promise) ReferenceError: require is not defined

Workaround

Use import instead of require (example).

What's going on (I think)

Update -- see next post, but the following may be useful context

The bundles (in auspice-client/dist/) contain one require() statement coming from this line in the customisation code. These commonJS require() statements should be handled by (auspice's) webpack - in webpack v4 this was handled via a file-loader but v5 now uses asset modules. The relevant part of our webpack config is:

https://github.com/nextstrain/auspice/blob/a4487b59989270b860d2508636f09ebdbc50d0f8/webpack.config.js#L248-L251

Since the non-customised auspice code includes plenty of require statements which are correctly handled, this strongly implies the asset-module is not considering the customised code which lives outside of node_modules/auspice

A long time ago we had to specify which directories we wanted to include for the file-loader, this was removed by https://github.com/nextstrain/auspice/commit/c93e97b6c1c6211a3d6aaa1c9fd3818fa43b8c2b ~3 years ago. I think the solution is something similar - we have to specify to the asset-module to specifically include the files which are part of the customised code, and thus not in the auspice tree.

Solutions I've tried which don't work (otherwise this would be a PR)

_I've found it's easier to debug this within the context of nextstrain.org (see above) by modifying the <nextstrain.org>/node_modules/auspice/webpack.config.js file_

  1. I reinstated the directoriesToTransform:
  const directoriesToTransform = [
    path.join(__dirname, 'src'), // auspice src
  ];
  let extensionData;
  if (extensionPath) {
    const dir = path.resolve(__dirname, path.dirname(extensionPath));
    directoriesToTransform.push(dir);

  ...

        {
          test: /\.(gif|png|jpe?g|svg|woff2?|eot|otf|ttf)$/i,
          type: "asset/resource",
          include: directoriesToTransform
        },

This results in webpack warnings indicating that assets outside auspice's src aren't being included here such when they should (e.g. ERROR in ../typeface-lato/files/lato-latin-900italic.woff2 1:4 ... You may need an appropriate loader to handle this file type, currently no loaders are configured to process this file.)

  1. Attempted to expand the directoriesToTransform to include those kind of files:
  if (fs.existsSync(path.join(__dirname, "node_modules"))) {
    // global npm install or install from source
    directoriesToTransform.push(path.join(__dirname, "node_modules"))
  } else {
    directoriesToTransform.push(path.join(__dirname, "..", "node_modules"))
  }

(same error)

  1. Attempted to recreate what we removed in https://github.com/nextstrain/auspice/commit/c93e97b6c1c6211a3d6aaa1c9fd3818fa43b8c2b -- same error (unsurprised by this stage)

  2. I tried undoing https://github.com/nextstrain/auspice/commit/7892c598fa305c2f1ee21c9aab5be01f15063c28 and reverting to the file-loader (which is soon to be deprecated). I thought this would work, but same require error 🤔

What we're trying to do in auspice is slightly off the beaten track, and I remember this was tricky to get working. I can't explain why the new version of webpack worked for me in testing last week -- recreating that now results in the same require error.

jameshadfield commented 1 year ago

I just noticed that there is a processed asset of nextstrain-logo-small.png at auspice-client/dist/13fec0d41212fe68ae22.png (filename seems stable) and that file is referenced in the minimised bundle (see below). This happens without needing any of potential solutions listed above (however I first observed this exploring potential solution no. 4).

I'm not that familiar with minimisation (I prefer not to look at minimised code!) but I think I can piece together some parts of the bundled output (auspice-client/dist/auspice.chunk.89.bundle.0af73a36cc412d8f616b.js for me):

In non-minimized form the relevant code is:

var map = {
    "./config": 6348,
    "./config.json": 6348,
    "./languageSelector": 3795,
    "./languageSelector.js": 3795,
    "./navbar": 9696,
    "./navbar.js": 9696,
    "./nextstrain-logo-small.png": 85976,
    "./splash": 58230,
    "./splash.js": 58230
};
...
var logoPNG = require("./nextstrain-logo-small.png");
...
react__WEBPACK_IMPORTED_MODULE_0__.createElement("img", {
    alt: "splashPage",
    width: "40px",
    src: logoPNG
  })
...
/***/ 85976:
/***/ ((module, __unused_webpack_exports, __webpack_require__) => {

"use strict";
module.exports = __webpack_require__.p + "13fec0d41212fe68ae22.png";

/***/ }),

I can fix the :bug: via either of the following ways - the second being the typical way webpack "requires" assets and the way it is doing it for images within the auspice src tree.

P.S. make sure to gzip the bundle to see the change

- var logoPNG = require("./nextstrain-logo-small.png");
+ var logoPNG = __webpack_require__.p + "13fec0d41212fe68ae22.png"
- var logoPNG = require("./nextstrain-logo-small.png");
+ var logoPNG = __webpack_require__(85976)

I don't think i'll do any more today (friday afternoon). I'm unsure if this is a webpack bug or a configuration bug on our side.

victorlin commented 1 year ago

@jameshadfield sorry you had to go down this rabbit hole! Nice dissection of the minimized code.

Without doing anything on the Auspice side, this error can be patched by adapting the downstream code to work (in the nextstrain.org example, replacing require with import https://github.com/nextstrain/nextstrain.org/commit/8b0c6563727f2cb915eb1e752a68db1911d24a56). I'd do this in place of the other nextstrain.org changes you proposed above.

However, I agree that this seems like a Webpack bug since file-loader in Webpack v4 is documented to handle both import and require(), so asset/resource should as well.

jameshadfield commented 1 year ago

Using import is a good solution to this situation (and we should totally use it), but it's still frustrating as this limits the extension capabilities of auspice.

Note that asset/resource handles require() statements when used in the auspice source tree (see examples below), I think this :bug: exists because of the file location of the offending require() statement.

https://github.com/nextstrain/auspice/blob/8d1354652394f6818faab31bef0fcda171f9abbe/src/components/framework/fine-print.js#L13

https://github.com/nextstrain/auspice/blob/5f36a94102916879e8b75e4cb145df79011e7f2f/src/components/framework/footer-descriptions.js#L197

https://github.com/nextstrain/auspice/blob/adc6df36c5c36e37fb504274657249a7a1fea0ac/src/components/framework/spinner.js#L4

victorlin commented 1 year ago

There's a hint in the webpack output of auspice build.

Before https://github.com/nextstrain/nextstrain.org/commit/8b0c6563727f2cb915eb1e752a68db1911d24a56 (with require):

modules by path ../nextstrain.org/ 205 KiB (javascript) 23.4 KiB (asset)
  javascript modules 204 KiB
    modules by path ../nextstrain.org/auspice-client/customisations/ 41.4 KiB 4 modules
    modules by path ../nextstrain.org/node_modules/ 163 KiB 3 modules
  ../nextstrain.org/auspice-client/customisations/config.json 372 bytes [optional] [built] [code generated]
  ../nextstrain.org/auspice-client/customisations/nextstrain-logo-small.png 42 bytes (javascript) 23.4 KiB (asset) [optional] [built] [code generated]

After https://github.com/nextstrain/nextstrain.org/commit/8b0c6563727f2cb915eb1e752a68db1911d24a56 (with import):

modules by path ../nextstrain.org/ 205 KiB (javascript) 23.4 KiB (asset)
  javascript modules 204 KiB
    modules by path ../nextstrain.org/auspice-client/customisations/ 41.4 KiB 4 modules
    modules by path ../nextstrain.org/node_modules/ 163 KiB 3 modules
  ../nextstrain.org/auspice-client/customisations/config.json 372 bytes [optional] [built] [code generated]
  ../nextstrain.org/auspice-client/customisations/nextstrain-logo-small.png 42 bytes (javascript) 23.4 KiB (asset) [built] [code generated]

The only difference is the [optional] next to nextstrain-logo-small.png in the one that fails. I'm trying to reproduce this with a test app, but so far unsuccessful.

I currently don't know enough about webpack to understand this output. Might be a good reason to go through the documentation/tutorials more thoroughly.

victorlin commented 1 year ago

Since the non-customised auspice code includes plenty of require statements which are correctly handled, this strongly implies the asset-module is not considering the customised code which lives outside of node_modules/auspice

This is a fair point, but I want to understand better. How does Auspice differentiate between non-customised vs. customised code? From what I can tell, customised code defined by the auspice build --extend <extensionPath> parameter goes through these extra hoops:

  1. In webpack.config.js:
    1. @extensions is an alias to <extensionPath>
    2. process.env.EXTENSION_DATA is a globally available variable with the extension config file's contents as its value
  2. In extensions.js:
    1. Upon initial execution of this file, the const registry = (() => { … })() IIFE requires all the components from the extension config.
    2. required components are used in place of non-customised code, e.g. https://github.com/nextstrain/auspice/blob/a4487b59989270b860d2508636f09ebdbc50d0f8/src/components/navBar/index.js#L24-L25

Are those the only differences between the non-customised and customised logoPNG, meaning something in those extra hoops is causing this bug?

// from auspice/src/components/navBar/content.js
// non-customised, works fine
const logoPNG = require("../../images/logo-light.svg");

// from nextstrain.org/auspice-client/customisations/navbar.js
// customised through auspice build --extend, does not work
const logoPNG = require("./nextstrain-logo-small.png");
jameshadfield commented 1 year ago

Yup, that's right. Couple of notes that may or may not help:

victorlin commented 1 year ago

I've updated the issue description with more current repro steps and the known workaround.

I've also categorized on the project board as:

Since I've hit a brick wall at this point, and given the categorization, I'm going to backlog this and work on some other things for now.