Attempt to fix commonjs module resolution weirdness #1135
Conversation
| @@ -14746,8 +14746,10 @@ func (c *Checker) resolveESModuleSymbol(moduleSymbol *ast.Symbol, referencingLoc | |||
| symbol := c.resolveExternalModuleSymbol(moduleSymbol, dontResolveAlias) | |||
| if !dontResolveAlias && symbol != nil { | |||
| if !suppressInteropError && symbol.Flags&(ast.SymbolFlagsModule|ast.SymbolFlagsVariable) == 0 && ast.GetDeclarationOfKind(symbol, ast.KindSourceFile) == nil { | |||
| compilerOptionName := core.IfElse(c.moduleKind >= core.ModuleKindES2015, "allowSyntheticDefaultImports", "esModuleInterop") | |||
| c.error(referencingLocation, diagnostics.This_module_can_only_be_referenced_with_ECMAScript_imports_Slashexports_by_turning_on_the_0_flag_and_referencing_its_default_export, compilerOptionName) | |||
| if ast.GetDeclarationOfKind(symbol, ast.KindJSExportAssignment) == nil { | |||
There was a problem hiding this comment.
The original code didn't do something like this; @sandersn is this because the JS the external module symbol isn't the source file anymore?
There was a problem hiding this comment.
Also, I think this check could just be in the previous if statement? (But I don't know if it's right in any case.)
There was a problem hiding this comment.
I don't understand how this fix is related to the previous code or to the repro in the bug. If you want to know if the file is a commonjs module, there's still CommonJSModuleIndicator in Corsa. But there's nothing about that in the Strada code either.
There was a problem hiding this comment.
Currently the repro returns nil for ast.GetDeclarationOfKind(symbol, ast.KindSourceFile) but (I think) is valid code.
The bug is that, in the repro,
import { foo } from "./shared.vars";fails while
import f from "./shared.vars";
const { foo } = f;is fine.
In the repro condition the node has type KindJSExportAssignment, so this check prevents the error-emitting block from getting entered.
There was a problem hiding this comment.
I need to know what the original code does; the symbol it creates is going to be different, and weirder, than the Corsa one, but it avoids this error some other way.
There was a problem hiding this comment.
Yep, @jakebailey is right on both counts. In Strada module.exports merges with the source file, but export= does not. It is flagged as Property. In Corsa, that's also how module.exports behaves.
And it does make sense to add this check to the previous if since it is one of the exemptions to the error.
| //// [tests/cases/compiler/commonJsModule.ts] //// | ||
|
|
||
| === shared.vars.js === | ||
| const foo = ['bar', 'baz']; |
There was a problem hiding this comment.
you don't need this new test since existing tests show that the fix works
| @@ -14746,8 +14746,10 @@ func (c *Checker) resolveESModuleSymbol(moduleSymbol *ast.Symbol, referencingLoc | |||
| symbol := c.resolveExternalModuleSymbol(moduleSymbol, dontResolveAlias) | |||
| if !dontResolveAlias && symbol != nil { | |||
| if !suppressInteropError && symbol.Flags&(ast.SymbolFlagsModule|ast.SymbolFlagsVariable) == 0 && ast.GetDeclarationOfKind(symbol, ast.KindSourceFile) == nil { | |||
| compilerOptionName := core.IfElse(c.moduleKind >= core.ModuleKindES2015, "allowSyntheticDefaultImports", "esModuleInterop") | |||
| c.error(referencingLocation, diagnostics.This_module_can_only_be_referenced_with_ECMAScript_imports_Slashexports_by_turning_on_the_0_flag_and_referencing_its_default_export, compilerOptionName) | |||
| if ast.GetDeclarationOfKind(symbol, ast.KindJSExportAssignment) == nil { | |||
There was a problem hiding this comment.
Yep, @jakebailey is right on both counts. In Strada module.exports merges with the source file, but export= does not. It is flagged as Property. In Corsa, that's also how module.exports behaves.
And it does make sense to add this check to the previous if since it is one of the exemptions to the error.
Maybe fixes #1054
I don't know whether this is the right fix, but @sandersn presumably can close if it's incorrect.