Conditional assignments to exports should not mark a js file as a commonjs module

[weswigham]

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 @sandersn @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.

[andrewbranch]

I definitely thought there was some isTopLevel filtering in the export-related code paths 🤔

[andrewbranch]

Related: playing around with this in VS Code, I get this in the console:

Error: Debug Failure. Invalid cast. The supplied value [object Object] did not pass the test 'isPropertyAccessExpression'.
    at Object.cast (tsserver.js:1463:25)
    at getSpecialPropertyExport (tsserver.js:108950:77)
    at getExport (tsserver.js:108926:32)
    at Object.getImportOrExportSymbol (tsserver.js:108882:53)
    at getImportOrExportReferences (tsserver.js:110321:56)
    at getReferencesAtLocation (tsserver.js:110253:17)
    at getReferencesInContainer (tsserver.js:110197:21)
    at getReferencesInContainerOrFiles (tsserver.js:109756:21)
    at getReferencedSymbolsForSymbol (tsserver.js:109747:21)
    at Object.getReferencedSymbolsForNode (tsserver.js:109550:34)
    at Object.getReferenceEntriesForNode (tsserver.js:109277:58)
    at getSemanticDocumentHighlights (tsserver.js:107967:57)

[object Object]

😦

[andrewbranch]

@weswigham

Conditional assignments to exports should not mark a js file as a commonjs module

Is it the fact that it’s conditional, or the fact that exports is a local? This code has the exact same behavior as if you replaced exports["AST_" + type] = ctor; with exports.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]

Right, exactly - there's a few conditions I think we need to fix.

1. Computed names on an exports that we can't bind to any name probably shouldn't make the file as a module (eg, exports["" + foo])
2. We shouldn't be scanning for export assignments in non-IIFE function bodies (since we can't actually check to what the exports reference refers to, for one, and for two, the function may never be executed - so the signal here is quite weak).

[andrewbranch]

We shouldn't be scanning for export assignments in non-IIFE function bodies

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]

Counterpoint: Bundler-packed code is often more complex than a simple IIFE: aframe/dist/aframe-master.js is a good example.

[weswigham]

Countercounterpoint: bundler-packed code usually is bundling multiple modules, not all of which's exports are top-level and visible to consumers.

[andrewbranch]

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]

We shouldn't be scanning for export assignments in non-IIFE function bodies (since we can't actually check to what the exports reference refers to, for one, and for two, the function may never be executed - so the signal here is quite weak).

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.