Repository navigation
Conditional assignments to exports should not mark a js file as a commonjs module #33765
Description
The addition of element accesses on exports broke the uglify-js user suite test. This is a minimal repro:
Code
// @noEmit: true
// @checkJs: true
// @allowJs: true
// @strict: true
// @filename: file1.js
// this file _should_ be a global file
var GlobalThing = { x: 12 };
/**
* @param {*} type
* @param {*} ctor
* @param {*} exports
*/
function f(type, ctor, exports) {
if (typeof exports !== "undefined") {
exports["AST_" + type] = ctor;
}
}
// @filename: ref.js
GlobalThing.x;Expected behavior:
file1.js is treated as a globally scoped file and ref.js typechecks.
Actual behavior:
Err in ref.js, no referenced found to GlobalThing in file1.js because file1.js is treated like a module.
cc Nathan Shively-Sanders (@sandersn) Andrew Branch (@andrewbranch) While this error only appeared once we started to recognize element access export assignments (even if we can't bind them because they're computed names, for some reason), that we flag a reference to a variable named exports within a function body (and a conditional!) as a module also seems questionable.
Activity
andrewbranch commentedon Oct 2, 2019
I definitely thought there was some
isTopLevelfiltering in the export-related code paths 🤔andrewbranch commentedon Oct 2, 2019
Related: playing around with this in VS Code, I get this in the console:
😦
andrewbranch commentedon Oct 2, 2019
Wesley Wigham (@weswigham)
Is it the fact that it’s conditional, or the fact that
exportsis a local? This code has the exact same behavior as if you replacedexports["AST_" + type] = ctor;withexports.foo = ctor. They both mark the file as a module. The element access isn’t really relevant here; it’s just what made us notice the bug.weswigham commentedon Oct 2, 2019
Right, exactly - there's a few conditions I think we need to fix.
exportsthat we can't bind to any name probably shouldn't make the file as a module (eg,exports["" + foo])exportsreference refers to, for one, and for two, the function may never be executed - so the signal here is quite weak).andrewbranch commentedon Oct 3, 2019
This seems like a good idea, but would be a breaking change... hopefully not too many users would be affected by it, but I think that part should probably wait for 3.8.
sandersn commentedon Oct 3, 2019
Counterpoint: Bundler-packed code is often more complex than a simple IIFE: aframe/dist/aframe-master.js is a good example.
weswigham commentedon Oct 3, 2019
Countercounterpoint: bundler-packed code usually is bundling multiple modules, not all of which's exports are top-level and visible to consumers.
andrewbranch commentedon Oct 17, 2019
The 3.7 part of this was done in #33784. The remaining bit is a breaking change so I’m moving the milestone to 3.8.
andrewbranch commentedon Nov 1, 2019
Tried this in #34805, and it turns out we have a number of tests that assert that we should scan for export assignments all over the place. This is JavaScript, after all, so I would wager that users would rather us just trust them that their nested exports are legitimate. Let’s keep the existing behavior until/unless it causes a problem and people ask for something different.