Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
jameshadfield opened this issue Dec 9, 2022 · 7 comments
Labels
bug Something isn't working

Comments

@jameshadfield
Copy link
Member

jameshadfield commented Dec 9, 2022

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:

auspice/webpack.config.js

Lines 248 to 251 in a4487b5

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

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 c93e97b ~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 c93e97b -- same error (unsurprised by this stage)

  2. I tried undoing 7892c59 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 jameshadfield added the bug Something isn't working label Dec 9, 2022
@jameshadfield
Copy link
Member Author

jameshadfield commented Dec 9, 2022

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):

  • There's some mapping set up early: "./nextstrain-logo-small.png":85976
  • Later on we have 85976: (e,t,n)=>{"use strict";e.exports=n.p+"13fec0d41212fe68ae22.png"}
  • 85976 is not referred to again (in this bundle or any others)
  • However we still have var g=require("./nextstrain-logo-small.png")

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 🐛 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.

@jameshadfield jameshadfield changed the title Webpack asset/resource module doesn't include client-side extensions Webpack asset/resource module incorrectly handling client-side extensions Dec 9, 2022
@victorlin
Copy link
Member

@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 nextstrain/nextstrain.org@8b0c656). 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
Copy link
Member Author

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 🐛 exists because of the file location of the offending require() statement.

const logoPNG = require("../../images/favicon.png");

<img alt="gisaid-logo" width="120" src={require("../../images/gisaid-logo.png")}/>

const nextstrainLogo = require("../../images/nextstrain-logo-small.png");

@victorlin
Copy link
Member

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

Before nextstrain/nextstrain.org@8b0c656 (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 nextstrain/nextstrain.org@8b0c656 (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
Copy link
Member

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.
      const Content = hasExtension("navbarComponent") ?
      getExtension("navbarComponent") : AuspiceNavBar;

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");

@huddlej huddlej moved this from New to In Progress in Nextstrain planning (archived) Dec 14, 2022
@jameshadfield
Copy link
Member Author

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

  • @auspice was intended to be useful for the customisations to import from the auspice src - e.g. in auspice.us we have import { parseMarkdown } from "@auspice/util/parseMarkdown";, @extensions is only used in the registry IIFE as far as I'm aware.
  • Webpack is clearly traversing the files in @extensions - they're bundled, minimised, import statements handled, and the require statement mostly handled!

@victorlin victorlin changed the title Webpack asset/resource module incorrectly handling client-side extensions Webpack asset/resource module incorrectly handling client-side extensions using require (ReferenceError: require is not defined) Dec 15, 2022
@victorlin victorlin moved this from In Progress to Backlog in Nextstrain planning (archived) Dec 15, 2022
@victorlin
Copy link
Member

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

I've also categorized on the project board as:

  • Type: 2 - Usability w/ workaround
  • Priority: 2 - A pain
  • Likelihood: 1 - Few users

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
No open projects
Status: Backlog
Development

No branches or pull requests

2 participants