-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Include reexported names in list of exported names #38809
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
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.
I‘m not sure what happend... but LGTM...
Ping @rbuckton ❤️ |
@@ -339,37 +339,6 @@ namespace ts { | |||
} | |||
} | |||
|
|||
for (const externalImport of moduleInfo.externalImports) { |
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.
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?
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.
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).
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.
Looks good, assuming we have adequate coverage for system modules, per my other comment.
@RyanCavanaugh should we backport this change to 3.9? |
Yes please 🙏😅 |
@weswigham I think you may have missed a test baseline. PR builds are currently failing:
It looks like the diff is due to this change. |
Semantic merge conflicts shakes fist |
Fixes #38530