Fix CommonJs modules #7249
Fix CommonJs modules #7249
Conversation
| @@ -278,9 +278,11 @@ namespace ts { | |||
| function declareSymbol(symbolTable: SymbolTable, parent: Symbol, node: Declaration, includes: SymbolFlags, excludes: SymbolFlags): Symbol { | |||
| Debug.assert(!hasDynamicName(node)); | |||
|
|
|||
| const isJsModuleExport = node.kind === SyntaxKind.BinaryExpression ? getSpecialPropertyAssignmentKind(node) === SpecialPropertyAssignmentKind.ModuleExports : false; | |||
There was a problem hiding this comment.
Shouldn't this be conditional on being in a JS file?
There was a problem hiding this comment.
Also, just
node.kind === SyntaxKind.BinaryExpression && getSpecialPropertyAssignmentKind(node) === SpecialPropertyAssignmentKind.ModuleExports
There was a problem hiding this comment.
No need to check node.kind === SyntaxKind.BinaryExpression; getSpecialPropertyAssignmentKind already does this (and returns .None)
There was a problem hiding this comment.
I added the JS file check to getSpecialPropertyAssignmentKind, as these only apply to JS files.
|
Looks OK overall. We should have a few compiler baselines for this scenario so we can get symbol baselines (since these are the easiest way to detect regressions in these sorts of features). |
|
Note: Just confirmed this fixes the issue raised in #7000 also.: You can import React as a namespace using the default CommonJS module type:
Or if using "system", the synthetic default also works:
|
|
👍 |
| @@ -1428,7 +1430,7 @@ namespace ts { | |||
| function bindModuleExportsAssignment(node: BinaryExpression) { | |||
| // 'module.exports = expr' assignment | |||
| setCommonJsModuleIndicator(node); | |||
| bindExportAssignment(node); | |||
| declareSymbol(file.symbol.exports, file.symbol, node, SymbolFlags.Property | SymbolFlags.Export | SymbolFlags.ValueModule, SymbolFlags.None); | |||
There was a problem hiding this comment.
Is there a reason you're setting with SymbolFlags.Property?
There was a problem hiding this comment.
In part because 'module.exports' is a property, but mostly because there was some code path that was crapping out without an expected flag set, of which property seemed appropriate (the exact code path escapes me, as I wrote this over a week ago, and there were a lot of code paths).
Happy to be corrected on my usage of the mystic arts of SymbolFlags however.
There was a problem hiding this comment.
shouldn't this be just SymbolFlags.Alias?
There was a problem hiding this comment.
I went down that road and it was a can of worms. I can't remember all the issues I hit, many related to resolveAlias, but one of them was the getTargetOfAlias function has a set of explicit SyntaxKinds it expects. It also expects to be able to resolve a target declaration, whereas this is a binaryExpression setting a property from an expression (e.g. it could be module.exports = someCall() - as I have a test-case for).
There was a problem hiding this comment.
i see now that would not work. but we should do it if the right hand side is an identified. this allows us to get a class from JS for instance in TS, and we have this mostly working today, you will just need to add one additional clause in getTargetOfAliasDeclaration.
if it is not, i would give this Property | ValueModule seems reasonable. not sure i see why we need the Export one though.
Ports CommonJS module fix #7249
|
I wish the "export = " pattern was deprecated. If TS is a superset of JS, then I wonder what benefit "export=" pattern offers to JS? After CommonJS, and AMD tried to fill the gap, now we have es2015 modules. We don't need "export=" pattern. |
|
For example, a module that exports a single function - that you want to be made available via |
|
Cross-referencing: Using this interop in the package could break at consumer level. |
This fix allows ES2015 module syntax (namespace imports and import lists) to work with:
a) JavaScript modules using the "module.exports = " pattern
b) TypeScript modules using the "export = " pattern
Ping @RyanCavanaugh and @mhegazy