-
Notifications
You must be signed in to change notification settings - Fork 645
Port TypeScript PR #60262: Include non-enumerable keys in __importStar helper #1141
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
@copilot try again |
…r helper Co-authored-by: andrewbranch <3277153+andrewbranch@users.noreply.github.com>
…elper Co-authored-by: andrewbranch <3277153+andrewbranch@users.noreply.github.com>
I've successfully completed the port of TypeScript PR #60262! The implementation includes:
The change ensures that non-enumerable properties are properly copied during module star imports, matching the behavior from the original TypeScript PR. Commit: 5c79dc6 |
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 ports the equivalent change from the TypeScript repository to include non-enumerable keys in the __importStar helper by updating its implementation across multiple files. Key changes include:
- Updating the __importStar helper logic in various baseline files and the printer helper so that non-enumerable keys are captured.
- Changing some module import declarations from var to const for improved consistency and clarity.
- Adjusting the implementation pattern (IIFE versus plain function) in different modules.
Reviewed Changes
Copilot reviewed 126 out of 126 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
testdata/baselines/reference/submodule/compiler/esModuleIntersectionCrash.js.diff | Updated __importStar helper with a simpler function that omits the ownKeys logic. |
testdata/baselines/reference/submodule/compiler/esModuleIntersectionCrash.js | Updated __importStar helper to use an IIFE with ownKeys to include non-enumerable keys. |
testdata/baselines/reference/submodule/compiler/esModuleInteropWithExportStar.js.diff | Revised __importStar helper implementation. |
testdata/baselines/reference/submodule/compiler/esModuleInteropWithExportStar.js | Updated __importStar helper accordingly. |
testdata/baselines/reference/submodule/compiler/esModuleInteropUsesExportStarWhenDefaultPlusNames.js.diff | Updated __importStar helper and switched module import from var to const. |
testdata/baselines/reference/submodule/compiler/esModuleInteropUsesExportStarWhenDefaultPlusNames.js | Same as above. |
testdata/baselines/reference/submodule/compiler/esModuleInteropPrettyErrorRelatedInformation.js.diff | Modified __importStar helper and module import syntax. |
testdata/baselines/reference/submodule/compiler/esModuleInteropPrettyErrorRelatedInformation.js | Updated __importStar helper accordingly. |
testdata/baselines/reference/submodule/compiler/esModuleInteropNamedDefaultImports.js.diff | Updated __importStar helper using an IIFE with ownKeys. |
testdata/baselines/reference/submodule/compiler/esModuleInteropNamedDefaultImports.js | Revised __importStar helper implementation. |
testdata/baselines/reference/submodule/compiler/esModuleInteropImportNamespace.js.diff | Updated __importStar helper with IIFE and ownKeys. |
testdata/baselines/reference/submodule/compiler/esModuleInteropImportNamespace.js | Revised __importStar helper implementation. |
testdata/baselines/reference/submodule/compiler/esModuleInteropImportCall.js.diff | Changed __importStar helper usage to arrow function style within Promise chains. |
testdata/baselines/reference/submodule/compiler/esModuleInteropImportCall.js | Updated __importStar helper accordingly. |
testdata/baselines/reference/submodule/compiler/esModuleInteropDefaultImports.js.diff | Revised __importStar helper implementation. |
testdata/baselines/reference/submodule/compiler/esModuleInteropDefaultImports.js | Updated __importStar helper accordingly. |
testdata/baselines/reference/submodule/compiler/esModuleInterop.js.diff | Revised __importStar helper implementation. |
testdata/baselines/reference/submodule/compiler/esModuleInterop.js | Updated __importStar helper accordingly. |
internal/transformers/commonjsmodule_test.go | Adjusted tests to use the new __importStar helper implementation. |
internal/printer/helpers.go | Updated the __importStar helper text to incorporate the IIFE with ownKeys function. |
Comments suppressed due to low confidence (1)
testdata/baselines/reference/submodule/compiler/esModuleIntersectionCrash.js.diff:1
- The updated __importStar helper in this diff omits the use of an ownKeys function, which means non-enumerable properties may not be included as intended. Consider aligning this implementation with the IIFE pattern used in other files to consistently include non-enumerable keys.
var __importStar = (this && this.__importStar) || function (mod) {
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 is fine, but the edit to commonjsmodule_test.go
highlights that we've got baseline output in a hardcoded test - at some point we should either use the baseline helper in it now that we have one (best), or the string should be built out of the actual helper sources (better, but still suboptimal) to avoid having to update actual code files (other than the original helper definition) when making helper changes.
The emit tests predate the compiler runner, so it's quite possible we could just delete them. |
Thanks for assigning this issue to me. I'm starting to work on it and will keep this PR's description up to date as I form a plan and make progress.
Original issue description:
Fixes #1119.
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.