require is unavailable when scope hoisting enabled in Parcel 2

[leighmcculloch]

🐛 bug report

Building for production with Parcel 2 results in some dependencies failing to require their subdependencies, specifically I am seeing this with jspreadsheet-ce failing to require its dependency jsuites.

🎛 Configuration (.babelrc, package.json, cli command)

{
  "name": "test",
  "version": "0.0.0",
  "description": "",
  "browserslist": [
    "last 1 Chrome version"
  ],
  "scripts": {
    "dev": "parcel index.html",
    "build": "parcel build index.html --no-source-maps --no-minify"
  },
  "author": "Leigh McCulloch",
  "devDependencies": {
    "cssnano": "^4.1.10",
    "postcss": "^8.2.6",
    "sass": "^1.32.7"
  },
  "dependencies": {
    "jspreadsheet-ce": "^4.6.0",
    "jszip": "^3.6.0",
    "parcel": "^2.0.0-beta.1"
  }
}

🤔 Expected Behavior

I expected jspreadsheet-ce to require jsuites and to see no errors.

😯 Current Behavior

I see this error in the browser:

main.b3eddd50.js:9702 Uncaught TypeError: Cannot read property 'contextmenu' of undefined
    at Object.obj.createTable (main.b3eddd50.js:9702)
    at Object.obj.prepareTable (main.b3eddd50.js:9540)
    at Object.obj.init (main.b3eddd50.js:16060)
    at jexcel (main.b3eddd50.js:16339)
    at window.onload (main.b3eddd50.js:23681)

The line that error fails on indicates that jSuites is undefined:

jSuites.contextmenu(obj.contextMenu, {

I believe the error is being caused by these lines of code in the jspreadsheet-ce jexcel.js file.

if (! jSuites && typeof(require) === 'function') {
    var jSuites = require('jsuites');
    require('jsuites/dist/jsuites.css');
}

Ref: https://github.com/jspreadsheet/ce/blob/v4.6.0/dist/jexcel.js#L10-L13

Parcel 2 compiles those lines of code to:

    if (!jSuites && typeof require == "function") {
      var jSuites = $c843483dbe7a5b584d1a976ca0f236f9$init();
    }

When I debug those ☝🏻 lines of code I find that require is undefined.

If I disable scope hoisting with --no-scope-hoist, then require has a value and is no longer undefined.

Something to do with how the scope hoisting is being performed is causing require to no longer be available.

💁 Possible Solution

The fact that disabling scope hoisting with --no-scope-hoist causes require to be defined suggests that require is not being defined or passed into the hoisted scope appropriately. I can't tell if this is an idiosyncracy of the jspreadsheet-ce package, or if this is a bug in Parcel 2.

🔦 Context

I'm using jspreadsheet-ce in a project that was not using Parcel, and was importing its dependencies directly from unpkg.com. I'm migrating to using Parcel 2 and this is the only issue I have.

💻 Code Sample

https://github.com/leighmcculloch/parcel-bundler--parcel--issue-5883

🌍 Your Environment

Software	Version(s)
Parcel	2.0.0-beta.1
Node	v15.9.0
npm/Yarn	npm 7.5.3
Operating System	Ubuntu 20.04.1 LTS (Code Name: Focal)

[leighmcculloch]

The documentation for Parcel 2 suggests that scope hoisting is intended to work in the case that a require is called from inside a function or inside an if statement as is happening in jspreadsheet-ce.

If an asset is imported conditionally (or generally in a try/catch, a function an if statement) using CommonJS require (this isn't possible with the ESM syntax), we cannot add it into the top-level scope because its content should only be execute when it is actually required.

Ref: https://v2.parceljs.org/features/scope-hoisting/#wrapped-assets

[mischnic]

The problem seems to be that typeof require == "function" isn't replaced for the browser when eval exists in that file.

const b = require("./b");
console.log(b())


// b.js
if (typeof require === "function") {
    var jSuites = require("./c.js");
}
console.log(jSuites);
let x = () => {
    return eval("1");
};
module.exports = x;


// c.js
module.exports = "c";

[leighmcculloch]

Ah interesting. Does it have something to do with these lines of code? I'm struggling to see how the attribute that get set here would prevent the replacement from happening. Is it the path.stop()?

parcel/packages/shared/scope-hoisting/src/hoist.js

Lines 125 to 138 in b70830d

	CallExpression(path) {
	// If we see an `eval` call, wrap the module in a function.
	// Otherwise, local variables accessed inside the eval won't work.
	let callee = path.node.callee;
	if (
	isIdentifier(callee) &&
	callee.name === 'eval' &&
	!path.scope.hasBinding('eval', true)
	) {
	asset.meta.isCommonJS = true;
	shouldWrap = true;
	path.stop();
	}
	},

[mischnic]

Looks like it's because of this condition:

parcel/packages/shared/scope-hoisting/src/hoist.js

Lines 539 to 548 in 72ce767

	// Replace `typeof module` with "object"
	if (
	path.node.operator === 'typeof' &&
	isIdentifier(path.node.argument) &&
	TYPEOF[path.node.argument.name] &&
	!path.scope.hasBinding(path.node.argument.name) &&
	!path.scope.getData('shouldWrap')
	) {
	path.replaceWith(t.stringLiteral(TYPEOF[path.node.argument.name]));
	}

shouldWrap should only prevent the replacement of typeof module, typeof require should still be replaced.

[leighmcculloch]

@mischnic So there needs to be an extra condition in the expression in the if that checks that what is being typeof'd is module? I'm thinking path.node.argument == "module". What do you think?

[mischnic]

Yeah, something like this should work:

       !path.scope.hasBinding(path.node.argument.name) &&
-      !path.scope.getData('shouldWrap')
+      !(path.scope.getData('shouldWrap') && path.node.argument.name === 'module')