Handling irregular `require` statements

[tjcouch-sil]

Hi again! 👋

As I mentioned before, thanks for your work on this project! 🙂

Today I used patch-package to patch import-manager@0.4.2 for the project I'm working on (source code for the plugin configuration here).

It seems require statements that don't match the expected pattern of declaration and instantiation in one expression (e.g. typeof window<"u"&&typeof require<"u"&&(window.Buffer=require("buffer/").Buffer);) cause an error that closes the program and fails to build:

error during build:
TypeError: Cannot read properties of undefined (reading 'at')
    at #cjsNodeToUnit (file:///C:/Users/tj_co/source/repos/crowd.bible-extension-paranext/node_modules/import-manager/src/core.js:403:46)
    at file:///C:/Users/tj_co/source/repos/crowd.bible-extension-paranext/node_modules/import-manager/src/core.js:147:57
    at c (file:///C:/Users/tj_co/source/repos/crowd.bible-extension-paranext/node_modules/acorn-walk/dist/walk.mjs:75:7)
    at Object.skipThrough (file:///C:/Users/tj_co/source/repos/crowd.bible-extension-paranext/node_modules/acorn-walk/dist/walk.mjs:180:37)
    at c (file:///C:/Users/tj_co/source/repos/crowd.bible-extension-paranext/node_modules/acorn-walk/dist/walk.mjs:73:22)
    at base.UnaryExpression.base.UpdateExpression (file:///C:/Users/tj_co/source/repos/crowd.bible-extension-paranext/node_modules/acorn-walk/dist/walk.mjs:367:3)
    at c (file:///C:/Users/tj_co/source/repos/crowd.bible-extension-paranext/node_modules/acorn-walk/dist/walk.mjs:73:22)
    at Object.skipThrough (file:///C:/Users/tj_co/source/repos/crowd.bible-extension-paranext/node_modules/acorn-walk/dist/walk.mjs:180:37)
    at c (file:///C:/Users/tj_co/source/repos/crowd.bible-extension-paranext/node_modules/acorn-walk/dist/walk.mjs:73:22)
    at base.BinaryExpression.base.LogicalExpression (file:///C:/Users/tj_co/source/repos/crowd.bible-extension-paranext/node_modules/acorn-walk/dist/walk.mjs:370:3)
dep-79892de8.js:12548
Process exited with code 1

Essentially, it seems that core.js's #cjsNodeToUnit assumes the acorn node will be a straight-forward expression with declaration and instantiation without any complexity (e.g. var name = require('mod');). However, I just received the error when #cjsNodeToUnit was trying to parse the node when the code variable was set to typeof window<"u"&&typeof require<"u"&&(window.Buffer=require("buffer/").Buffer); (I think some dependency of mine was trying to polyfill Buffer). I stringified the node, whose contents follow in the dropdown:

JSON.stringify(node, null, 2)

{
  "type": "ExpressionStatement",
  "start": 21948,
  "end": 22029,
  "expression": {
    "type": "LogicalExpression",
    "start": 21948,
    "end": 22028,
    "left": {
      "type": "LogicalExpression",
      "start": 21948,
      "end": 21985,
      "left": {
        "type": "BinaryExpression",
        "start": 21948,
        "end": 21965,
        "left": {
          "type": "UnaryExpression",
          "start": 21948,
          "end": 21961,
          "operator": "typeof",
          "prefix": true,
          "argument": {
            "type": "Identifier",
            "start": 21955,
            "end": 21961,
            "name": "window"
          }
        },
        "operator": "<",
        "right": {
          "type": "Literal",
          "start": 21962,
          "end": 21965,
          "value": "u",
          "raw": "\"u\""
        }
      },
      "operator": "&&",
      "right": {
        "type": "BinaryExpression",
        "start": 21967,
        "end": 21985,
        "left": {
          "type": "UnaryExpression",
          "start": 21967,
          "end": 21981,
          "operator": "typeof",
          "prefix": true,
          "argument": {
            "type": "Identifier",
            "start": 21974,
            "end": 21981,
            "name": "require"
          }
        },
        "operator": "<",
        "right": {
          "type": "Literal",
          "start": 21982,
          "end": 21985,
          "value": "u",
          "raw": "\"u\""
        }
      }
    },
    "operator": "&&",
    "right": {
      "type": "AssignmentExpression",
      "start": 21988,
      "end": 22027,
      "operator": "=",
      "left": {
        "type": "MemberExpression",
        "start": 21988,
        "end": 22001,
        "object": {
          "type": "Identifier",
          "start": 21988,
          "end": 21994,
          "name": "window"
        },
        "property": {
          "type": "Identifier",
          "start": 21995,
          "end": 22001,
          "name": "Buffer"
        },
        "computed": false,
        "optional": false
      },
      "right": {
        "type": "MemberExpression",
        "start": 22002,
        "end": 22027,
        "object": {
          "type": "CallExpression",
          "start": 22002,
          "end": 22020,
          "callee": {
            "type": "Identifier",
            "start": 22002,
            "end": 22009,
            "name": "require"
          },
          "arguments": [
            {
              "type": "Literal",
              "start": 22010,
              "end": 22019,
              "value": "buffer/",
              "raw": "\"buffer/\""
            }
          ],
          "optional": false
        },
        "property": {
          "type": "Identifier",
          "start": 22021,
          "end": 22027,
          "name": "Buffer"
        },
        "computed": false,
        "optional": false
      }
    }
  }
}

It appears that there are no declarations in the statement, so node.declarations is undefined. When #cjsNodeToUnit accesses that property, it throws the error. acorn parses the expression with operator: '&&' and the actual assignment of the import (where it should have operator: '=') is nested in lefts and rights.

Fortunately for my current use case, I don't need to do any modification of this particular import statement, so I made a quick patch that essentially ignores the problematic line. However, I suggest a better long-term solution would be to traverse the node to find the operator: '=' and operate on that (accounting in any case for node.declarations being undefined).

Some other examples of different kinds of requires that likely don't match the expected pattern follow:

// Not tested, but I imagine declaring and assigning in two separate expressions would not work
var name; name = require('mod');
// Tested. If you run a require without any assignment or anything, it fails with the same error. These kinds of requires are useful for running modules' side effects
require('mod-with-side-effects');

Here is the diff that solved my problem:

diff --git a/node_modules/import-manager/cjs/import-manager.cjs b/node_modules/import-manager/cjs/import-manager.cjs
index 92314ec..56c5d3f 100644
--- a/node_modules/import-manager/cjs/import-manager.cjs
+++ b/node_modules/import-manager/cjs/import-manager.cjs
@@ -395,7 +395,6 @@ class ImportManagerUnitMethods {
  */
 
 
-
 class ImportManager {
 
     /**
@@ -520,6 +519,7 @@ class ImportManager {
                     
                     else if (part.type === "Identifier" && part.name === "require") {
                         const unit = this.#cjsNodeToUnit(node);
+                        if (!unit) return;
                         unit.id = cjsId ++;
                         unit.index = cjsIndex ++;
                         unit.hash = this.#makeHash(unit);
@@ -772,6 +772,7 @@ class ImportManager {
      * @returns {object} - Import Manager Unit Object.
      */
     #cjsNodeToUnit(node) {
+        if (!node || !node.declarations || node.declarations.length >= 0) return;
 
         const code = this.code.slice(node.start, node.end);
 
diff --git a/node_modules/import-manager/cjs/import-manager.cjs.map b/node_modules/import-manager/cjs/import-manager.cjs.map
index a701837..f890886 100644
--- a/node_modules/import-manager/cjs/import-manager.cjs.map
+++ b/node_modules/import-manager/cjs/import-manager.cjs.map
 SOURCEMAP OMITTED
diff --git a/node_modules/import-manager/src/core.js b/node_modules/import-manager/src/core.js
index 717400c..f3cf31f 100644
--- a/node_modules/import-manager/src/core.js
+++ b/node_modules/import-manager/src/core.js
@@ -145,6 +145,7 @@ class ImportManager {
                     
                     else if (part.type === "Identifier" && part.name === "require") {
                         const unit = this.#cjsNodeToUnit(node);
+                        if (!unit) return;
                         unit.id = cjsId ++;
                         unit.index = cjsIndex ++;
                         unit.hash = this.#makeHash(unit);
@@ -397,6 +398,7 @@ class ImportManager {
      * @returns {object} - Import Manager Unit Object.
      */
     #cjsNodeToUnit(node) {
+        if (!node || !node.declarations || node.declarations.length >= 0) return;
 
         const code = this.code.slice(node.start, node.end);

Thank you again for all your hard work on this awesome library!

This issue body was partially generated by patch-package.

[UmamiAppearance]

Hi. I will take a look into this later.

[tjcouch-sil]

Hi. I will take a look into this later.

No rush!! :) I'm not blocked because of the patch I made. Please take your time. Life is busy. Thanks for letting me know.

[UmamiAppearance]

So first, thanks for reporting this issue. As always your issue is very informative and shows a deep understanding of the plugin. I think the patch you made is great for a quick fix. If you don't mind you can make a pull request with your changes. I think it is a good idea to add this return conditions to dynamic and es6 as well.
If you don't have the time, I will add those changes my myself.

I suggest a better long-term solution would be to traverse the node to find the operator: '=' and operate on that (accounting in any case for node.declarations being undefined).

I agree. I will work on a real solution. As I barely use common js myself but I really need some research on the possible cases to cover.

[tjcouch-sil]

So first, thanks for reporting this issue. As always your issue is very informative and shows a deep understanding of the plugin. I think the patch you made is great for a quick fix. If you don't mind you can make a pull request with your changes. I think it is a good idea to add this return conditions to dynamic and es6 as well. If you don't have the time, I will add those changes my myself.

I suggest a better long-term solution would be to traverse the node to find the operator: '=' and operate on that (accounting in any case for node.declarations being undefined).

I agree. I will work on a real solution. As I barely use common js myself but I really need some research on the possible cases to cover.

Made PR #51 :) I didn't do any work on the others beside cjs, though, as I didn't see an obvious equivalent after a quick glance and don't have any test cases for it. Thanks again!

[UmamiAppearance]

Great. Thanks. No, I neither see an equivalent case for the others, although dynamic imports allow more creative constructions as well. I will just add it for the sake of completeness . If there is a situation, which isn't anticipated, this will prevent crashes.

[tjcouch-sil]

Hmm I wonder if it would be wise to log the code when this happens so people at least know what's going on and can figure things out from there in the future.

[UmamiAppearance]

Yes, it should say something like 'this is a bug' 😉
(I am BTW writing from my phone w/ auto correction)

[UmamiAppearance]

Resolved in #52
Left open for a real solution to come

[tjcouch-sil]

Thanks again for the quick response! I really appreciate all your hard work on this package. It's a life saver!