Consider the commonjs module indicator as a module indicator #18490
Conversation
… is effectively an external module
| function foo(name) { | ||
| var s = require("t/" + name); | ||
| } | ||
| define("a", ["require", "exports"], function (require, exports) { |
There was a problem hiding this comment.
hmm.. this does not look good. we should not mess up the module output if we can.
There was a problem hiding this comment.
not sure what to do though.. @rbuckton thoughts?
There was a problem hiding this comment.
This file has an explicit @module: amd BTW (this could be seen as an improvement). Our module emit and our global emit for commonjs are pretty much identical - the only difference is in how the typechecker sees the top-level scope.
|
It's also worth noting that our current output is actually correctly handled by including |
|
This is then a bigger change than originally intended. |
|
@mhegazy 👍 👎 Punt? Disable |
src/compiler/utilities.ts
Outdated
| @@ -444,7 +444,7 @@ namespace ts { | |||
| } | |||
|
|
|||
| export function isEffectiveExternalModule(node: SourceFile, compilerOptions: CompilerOptions) { | |||
| return isExternalModule(node) || compilerOptions.isolatedModules; | |||
| return isExternalModule(node) || compilerOptions.isolatedModules || !!node.commonJsModuleIndicator; | |||
There was a problem hiding this comment.
this seems like the best compromise here:
(!!node.commonJsModuleIndicator && getEmitModuleKind(compilerOptions) === ModuleKind.CommonJS)
There was a problem hiding this comment.
It seems reasonable. Though anyone with mixed-mode compilations targeting commonjs where they use require in some global files is going to get sad when their global files start getting transpiled as modules. Though why we even support mixed mode compilations in the first place...
There was a problem hiding this comment.
the other option is to do nothing and leave the current behavior.. which is broken on import helpers + js files (which could be a smaller group affected than by the proposed change)...
There was a problem hiding this comment.
mmmmmmm I think once limited to commonjs it's likely fine. The only difference other than helpers, which is what this aims to change, for module vs non-module would be that top level declarations are local to the file instead of global. And given that the commonjs module flag is only set in JS files, it's not like there'll be any potential interface or namespace merging going on; so it'd just be weather people expected the var x they declared in one file with a require to be visible in another file with a require..... except that's governed by isExternalModule and not isEffectiveExternalModule (since the latter only affects emit), so I'm pretty sure once limited to commonjs there's no obserable changes?
|
@mhegazy Accept like so? 👍 👎 |
within the module transformer. This causes us to pick up some js files with
requireorexport.as modules now, as can be seen in the baseline changes.Fixes #17192