Skip to content

Include reexported names in list of exported names#38809

Merged
weswigham merged 1 commit intomicrosoft:masterfrom
weswigham:hoist-reexport-names
Jun 15, 2020
Merged

Include reexported names in list of exported names#38809
weswigham merged 1 commit intomicrosoft:masterfrom
weswigham:hoist-reexport-names

Conversation

@weswigham
Copy link
Member

@weswigham weswigham commented May 27, 2020

Fixes #38530

Kingwl
Kingwl approved these changes May 28, 2020
View reviewed changes
Copy link
Contributor

@Kingwl Kingwl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I‘m not sure what happend... but LGTM...

@weswigham
Copy link
Member Author

weswigham commented Jun 13, 2020

Ping @rbuckton ❤️

@weswigham weswigham mentioned this pull request Jun 15, 2020
Closed
rbuckton
rbuckton reviewed Jun 15, 2020
View reviewed changes
src/compiler/transformers/module/system.ts
}
}

for (const externalImport of moduleInfo.externalImports) {
Copy link
Contributor

@rbuckton rbuckton Jun 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't see any tests affected that use SystemJS, so I'm not sure what effect (if any) this change made. Is it handled by the change in utilities.ts, or are we missing test cases?

Copy link
Member Author

@weswigham weswigham Jun 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This removed code duplicated the reexports preservation for systemjs. In effect, for systemjs only, we were tracking reexports in a second list. So, rather than duplicating the systemjs code into all the other module systems, I removed code from the systemjs resolver and let it rely on the single list having the reexports in it (since every module mode needs them now).

rbuckton
rbuckton approved these changes Jun 15, 2020
View reviewed changes
Copy link
Contributor

@rbuckton rbuckton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, assuming we have adequate coverage for system modules, per my other comment.

@weswigham
Copy link
Member Author

weswigham commented Jun 15, 2020

@RyanCavanaugh should we backport this change to 3.9?

@weswigham weswigham merged commit 4bda7ce into microsoft:master Jun 15, 2020
@ValentinH
Copy link

ValentinH commented Jun 16, 2020

Yes please 🙏😅

@rbuckton
Copy link
Contributor

rbuckton commented Jun 16, 2020

@weswigham I think you may have missed a test baseline. PR builds are currently failing:

 1) 
       
         unittests:: tsbuild:: inferredTypeFromTransitiveModule::
           tsc --b /src --verbose inferredTypeFromTransitiveModule:: reports errors in files affected by change in signature with isolatedModules serializedEdits
             serializedBuild
               Generates files matching the baseline:
     Error: The baseline file tsbuild/inferredTypeFromTransitiveModule/initial-build/reports-errors-in-files-affected-by-change-in-signature-with-isolatedModules.js has changed.
      at writeComparison (src/harness/harnessIO.ts:1384:23)
      at Object.runBaseline (src/harness/harnessIO.ts:1394:13)
      at Context.<anonymous> (src/testRunner/unittests/tsc/helpers.ts:148:30)
      at processImmediate (internal/timers.js:456:21)

It looks like the diff is due to this change.

@weswigham
Copy link
Member Author

weswigham commented Jun 16, 2020

Semantic merge conflicts shakes fist

@mjbvz mjbvz mentioned this pull request Jun 22, 2020
Closed
@weswigham weswigham mentioned this pull request Jun 23, 2020
Merged
@microsoft microsoft locked as resolved and limited conversation to collaborators Oct 21, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@RyanCavanaugh RyanCavanaugh Awaiting requested review from RyanCavanaugh

2 more reviewers

@rbuckton rbuckton rbuckton approved these changes

@Kingwl Kingwl Kingwl approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@weswigham weswigham

Labels

Author: Team

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Cannot redefine exports when exporting identifiers with the same name from different files

5 participants

@weswigham @ValentinH @rbuckton @Kingwl @typescript-bot