Handling irregular `require` statements #50

tjcouch-sil · 2023-06-08T19:43:26Z

Hi again! 👋

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

Today I used patch-package to patch [email protected] 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.

The text was updated successfully, but these errors were encountered:

UmamiAppearance · 2023-06-09T07:17:44Z

Hi. I will take a look into this later.

tjcouch-sil · 2023-06-09T11:05:35Z

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 · 2023-06-09T12:45:49Z

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 · 2023-06-09T13:59:56Z

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 · 2023-06-09T16:38:41Z

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 · 2023-06-09T16:59:52Z

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 · 2023-06-09T17:13:53Z

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

UmamiAppearance · 2023-06-10T10:46:08Z

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

tjcouch-sil · 2023-06-12T13:01:24Z

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

tjcouch-sil mentioned this issue Jun 9, 2023

Ignore irregular require statements e.g. ones that do not have a declaration statement #51

Merged

UmamiAppearance added the bug Something isn't working label Jun 9, 2023

UmamiAppearance mentioned this issue Jun 10, 2023

Irregular import statements - quick fix #52

Merged

UmamiAppearance removed the bug Something isn't working label Jun 10, 2023

UmamiAppearance added the enhancement New feature or request label Jun 10, 2023

UmamiAppearance changed the title ~~Error on Irregular require statements~~ Handling irregular require statements Jun 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handling irregular `require` statements #50

Handling irregular `require` statements #50

tjcouch-sil commented Jun 8, 2023

UmamiAppearance commented Jun 9, 2023 •

edited

Loading

tjcouch-sil commented Jun 9, 2023

UmamiAppearance commented Jun 9, 2023

tjcouch-sil commented Jun 9, 2023

UmamiAppearance commented Jun 9, 2023 •

edited

Loading

tjcouch-sil commented Jun 9, 2023

UmamiAppearance commented Jun 9, 2023 •

edited

Loading

UmamiAppearance commented Jun 10, 2023

tjcouch-sil commented Jun 12, 2023

Handling irregular require statements #50

Handling irregular require statements #50

Comments

tjcouch-sil commented Jun 8, 2023

UmamiAppearance commented Jun 9, 2023 • edited Loading

tjcouch-sil commented Jun 9, 2023

UmamiAppearance commented Jun 9, 2023

tjcouch-sil commented Jun 9, 2023

UmamiAppearance commented Jun 9, 2023 • edited Loading

tjcouch-sil commented Jun 9, 2023

UmamiAppearance commented Jun 9, 2023 • edited Loading

UmamiAppearance commented Jun 10, 2023

tjcouch-sil commented Jun 12, 2023

Handling irregular `require` statements #50

Handling irregular `require` statements #50

UmamiAppearance commented Jun 9, 2023 •

edited

Loading

UmamiAppearance commented Jun 9, 2023 •

edited

Loading

UmamiAppearance commented Jun 9, 2023 •

edited

Loading