Ignore noop dependencies

[fathyb]

Ignore dependencies in a falsy branch, example if(false) or if('').

This is needed when consuming Webpack generated UMD modules. For example in react-data-grid:

if (false) {
    var REACT_ELEMENT_TYPE = (typeof Symbol === 'function' &&
    Symbol.for &&
    Symbol.for('react.element')) ||
    0xeac7;

    var isValidElement = function(object) {
    return typeof object === 'object' &&
        object !== null &&
        object.$$typeof === REACT_ELEMENT_TYPE;
    };

    // By explicitly using `prop-types` you are opting into new development behavior.
    // http://fb.me/prop-types-in-prod
    var throwOnDirectAccess = true;
    module.exports = require('./factoryWithTypeCheckers')(isValidElement, throwOnDirectAccess);
}

Triggers this error :

C:\~\node_modules\react-data-grid\dist\react-data-grid.js:93:28: Cannot resolve dependency './factoryWithTypeCheckers'
  91 | a  // http://fb.me/prop-types-in-prod\react-data-
  92 |    var throwOnDirectAccess = true;
> 93 |    module.exports = require('./factoryWithTypeCheckers')(isValidElement, throwOnDirectAccess);
     |                             ^
  94 |  } else {
  95 |    // By explicitly using `prop-types` you are opting into new production behavior.
  96 |    // http://fb.me/prop-types-in-prod

---

Reported by Jindřich Pergler

[devongovett]

What about things that evaluate to false but aren't exactly false. e.g. if (process.env.NODE_ENV === "production")? The environment variable should already be replaced, so it would be if ("development" === "production") for example.

[mischnic]

Isn't that equivalent to this code (test/integration/optional-dep/index.js)?

if('') {
  require('if-falsy-optional-dep');
}

[fathyb]

@devongovett sounds good, would this only apply to process.env.NODE_ENV or other globals? I feel like __dirname, __filename and process.env is enough but your call 🙂

@mischnic I'm not sure to understand

[mischnic]

I was referring to @devongovett 's comment. "development" === "production" should evaluate to false just as '' does.

The replacement is already done here:

parcel/src/visitors/globals.js

Lines 21 to 31 in 6c3d34f

	MemberExpression(node, asset) {
	// Inline environment variables accessed on process.env
	if (matchesPattern(node.object, 'process.env')) {
	let key = types.toComputedKey(node);
	if (types.isStringLiteral(key)) {
	let val = types.valueToNode(process.env[key.value]);
	morph(node, val);
	asset.isAstDirty = true;
	asset.cacheData.env[key.value] = process.env[key.value];
	}
	}

[fathyb]

@mischnic Oh, gotcha. The replacement is done before src/visitors/globals is executed (collectDependencies is called before transform) so it does not evaluate it currently :

parcel/src/assets/JSAsset.js

Lines 90 to 109 in 3435c4c

	collectDependencies() {
	walk.ancestor(this.ast, collectDependencies, this);
	}
	async pretransform() {
	await babel(this);
	}
	async transform() {
	if (this.options.target === 'browser') {
	if (this.dependencies.has('fs') && FS_RE.test(this.contents)) {
	await this.parseIfNeeded();
	this.traverse(fsVisitor);
	}
	if (GLOBAL_RE.test(this.contents)) {
	await this.parseIfNeeded();
	walk.ancestor(this.ast, insertGlobals, this);
	}
	}

[jpergler]

Hi @fathyb, will there be any progress with this issue? Thank you.

[devongovett]

Updated to move environment variable inlining to a separate stage prior to dependency collection, and now evaluate the if statement test to determine whether it is true or false rather than only supporting literals.