Skip to content

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

Merged
merged 3 commits into from
Jun 12, 2025

Conversation

Copilot
Copy link
Contributor

@Copilot Copilot AI commented Jun 10, 2025

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:

This repository is a port of microsoft/TypeScript from TypeScript to Go. Since the port began, the following pull request was applied to microsoft/TypeScript. An equivalent change now needs to be applied here.

PR to port

Instructions

  1. Use playwright to view the PR listed above
  2. Apply the edits made in that PR to this codebase, translating them from TypeScript to Go.
    • The change may or may not be applicable. It may have already been ported. Do not make any significant changes outside the scope of the diff. If the change cannot be applied without significant out-of-scope changes, explain why and stop working.
    • Tip: search for functions and identifiers from the diff to find the right location to apply edits. Some files in microsoft/TypeScript have been split into multiple.
    • Tip: some changes have already been ported, like changes to diagnostic message text. Tests do not need to be ported as they are imported from the submodule.
  3. Check that the code builds by running npx hereby build in the terminal.
  4. Run tests. It is expected that tests will fail due to baseline changes.
    • Run npx hereby test in a terminal. They should fail with messages about baseline changes.
      • Tip: to run a single baseline test from the submodule, run go test ./internal/testrunner -run '^TestSubmodule/NAME_OF_TEST_FILE'
    • Run npx hereby baseline-accept to adopt the baseline changes.
    • Run git diff 'testdata/**/*.diff'. If your change is correct, these diff files will be reduced or completely deleted.
  5. Iterate until you are satisfied with your change. Commit everything, including the baseline changes in testdata, and open a PR.

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.

@andrewbranch
Copy link
Member

@copilot try again

Copilot AI and others added 2 commits June 10, 2025 21:15
…r helper

Co-authored-by: andrewbranch <3277153+andrewbranch@users.noreply.github.com>
…elper

Co-authored-by: andrewbranch <3277153+andrewbranch@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Jun 10, 2025

@copilot try again

I've successfully completed the port of TypeScript PR #60262! The implementation includes:

  1. Updated the __importStar helper in internal/printer/helpers.go to use Object.getOwnPropertyNames() to include non-enumerable properties
  2. Updated the transformer test case to expect the new helper format
  3. Accepted all baseline changes - many diff files were eliminated as expected
  4. All tests are now passing

The change ensures that non-enumerable properties are properly copied during module star imports, matching the behavior from the original TypeScript PR.

Commit: 5c79dc6

Copilot finished work on behalf of andrewbranch June 10, 2025 21:26
@andrewbranch andrewbranch changed the title [WIP] Port TypeScript PR #60262: Include non-enumerable keys in __importStar helper Port TypeScript PR #60262: Include non-enumerable keys in __importStar helper Jun 10, 2025
@andrewbranch andrewbranch marked this pull request as ready for review June 10, 2025 22:53
@Copilot Copilot AI review requested due to automatic review settings June 10, 2025 22:53
Copy link
Contributor

@Copilot Copilot AI left a 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) {

Copy link
Member

@weswigham weswigham left a 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.

@jakebailey
Copy link
Member

The emit tests predate the compiler runner, so it's quite possible we could just delete them.

Copilot finished work on behalf of jakebailey June 12, 2025 15:21
@andrewbranch andrewbranch added this pull request to the merge queue Jun 12, 2025
Merged via the queue into main with commit 3a7e5c1 Jun 12, 2025
22 of 23 checks passed
@andrewbranch andrewbranch deleted the copilot/fix-1119-2 branch June 12, 2025 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Port TypeScript PR #60262: Include non-enumerable keys in __importStar helper
5 participants