Skip to content
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

importHelpers: only import direct dependencies with ESM #41545

Conversation

d-fischer
Copy link

Fixes #41515.

Also orta told me to ping @rbuckton on this PR 😄

@typescript-bot typescript-bot added the For Backlog Bug PRs that fix a backlog bug label Nov 15, 2020
@d-fischer
Copy link
Author

This probably needs some perf testing because of the type change of emit helpers.

@d-fischer d-fischer changed the title Import helpers esm only direct dependencies importHelpers: only import direct dependencies with ESM Nov 15, 2020
@orta orta requested a review from rbuckton November 16, 2020 16:43
@sandersn sandersn requested a review from orta November 24, 2020 22:42
@d-fischer d-fischer force-pushed the importHelpers-esm-only-direct-dependencies branch from 214c8dd to a91000f Compare January 12, 2021 16:54
@d-fischer
Copy link
Author

When rebasing to fix the merge conflict this had, I noticed that this PR has no impact on any output anymore at the current time.

The main reason I created the PR is that I saw that the __spread helper depended on the __read helper. With CommonJS this isn't a problem because we require() the whole tslib anyway, and with importHelpers: false this isn't a problem because all helpers and its dependencies need to be inlined for the code to work. But with ESM (and importHelpers: true), we import the helpers directly, and we don't need to import their dependencies (because they internally call each other), but we currently do. This is what I wanted to change.

The reason this now seemingly has no impact is because __spread was deleted (#41523) and there is no other helper this has any impact on. __importStar and __exportStar both depend on __createBinding, but are only used with CommonJS, and __asyncGenerator and __asyncDelegator both depend on __await, but are both emitted only from code that also directly depends on __await itself.

For now, I removed the test because I can't imagine a test case that properly tests this with the current set of helpers.

...but the future!

There may be other helpers with internal dependencies in the future this does have impact on.

Now, how do we continue with this PR? Do we still merge it without tests to be future-proof for any new helpers this may have an impact on?
Alternatively, is there some edge case I'm overlooking, and this does currently have an impact we can test?

@sandersn
Copy link
Member

Unfortunately, we never finished reviewing this PR. It is pretty old now, so I'm going to close it to reduce the number of open PRs.

@sandersn sandersn closed this May 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Backlog Bug PRs that fix a backlog bug
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

importHelpers + module: es2015/esnext emits unused helper imports
4 participants