- 
                Notifications
    You must be signed in to change notification settings 
- Fork 726
Fix various module emit differences #1965
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes the shouldEmitImportEqualsDeclaration function to match the TypeScript reference implementation. The original ported version had different conditional logic that caused import equals declarations to be omitted in certain scenarios, leading to numerous emit differences in test baselines.
Key Changes
- Simplified the conditional logic in shouldEmitImportEqualsDeclarationto correctly emit import declarations in non-external modules
- Fixed the logic to use OR instead of AND for the conditions, matching TypeScript's behavior
Reviewed Changes
Copilot reviewed 82 out of 83 changed files in this pull request and generated 1 comment.
| File | Description | 
|---|---|
| internal/transformers/tstransforms/importelision.go | Corrected the shouldEmitImportEqualsDeclarationfunction logic to match TypeScript's reference implementation | 
| Multiple baseline .difffiles | Test baseline differences removed as the port now converges with stable TypeScript behavior | 
| Multiple baseline reference files | Updated baselines showing correct emission of import equals declarations | 
        
          
                testdata/baselines/reference/submodule/compiler/aliasBug.js.diff
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                testdata/baselines/reference/submodule/compiler/constEnums.js.diff
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                testdata/baselines/reference/submodule/compiler/importDeclWithClassModifiers.js.diff
              
                Outdated
          
            Show resolved
            Hide resolved
        
      | -exports.a = x.c; | ||
| -module.exports = x; | ||
| +Object.defineProperty(exports, "__esModule", { value: true }); | ||
| +Object.defineProperty(exports, "__esModule", { value: true }); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regression here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
    module x {
        interface c {
        }
    }
    export import a = x.c;
                      ~
!!! error TS2708: Cannot use namespace 'x' as a value.
                        ~
!!! error TS2694: Namespace 'x' has no exported member 'c'.
    export = x;
    ~~~~~~~~~~~
!!! error TS2309: An export assignment cannot be used in a module with other exported elements.
Hard to feel bad about breaking this one though...
        
          
                testdata/baselines/reference/submodule/conformance/exportsAndImports3-es6.js.diff
              
                Outdated
          
            Show resolved
            Hide resolved
        
      3715a7a    to
    6157576      
    Compare
  
    6157576    to
    982e906      
    Compare
  
    | if (symbol.Flags&(ast.SymbolFlagsFunction|ast.SymbolFlagsClass|ast.SymbolFlagsRegularEnum) == 0) && | ||
| state == ast.ModuleInstanceStateConstEnumOnly && | ||
| symbol.Flags&ast.SymbolFlagsConstEnumOnlyModule == 0 { | ||
| symbol.Flags |= ast.SymbolFlagsConstEnumOnlyModule | ||
| } else { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The old compiler used a boolean | undefined on every symbol for this. Corsa uses a flag but didn't make the behavior match the tristate. Doing this instead to avoid adding a Tristate to all symbols.
| v.Name().Clone(tx.Factory()), | ||
| )) | ||
| } else if ast.IsIdentifier(v.Name()) { | ||
| expression := tx.transformInitializedVariable(v) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The old compiler had this func instead which did something special with exports, unlike ConvertVariableDeclarationToAssignmentExpression which is new. Not extremely familiar, though.
Various things I noticed while looking at #1963