Skip to content

Fixes stack overflow when exporting a lot in commonjs #38994

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 2 commits into from
Sep 4, 2020

Conversation

ericanderson
Copy link
Contributor

Fixes #38691

I went with the simple solution of not generating long chains of BinaryExpressions rather than introducing a new type similar to CommaList to avoid large and potentially buggy code churn.

Please consider back porting to the next 3.9 release.

CC/ @weswigham @rbuckton @RyanCavanaugh

@ericanderson
Copy link
Contributor Author

If you fine folks want to introduce a BinaryExpressionChain I can attempt that in a separate PR for the next minor release but hoped we could put this into a patch release for 3.9.x.

statements,
createExpressionStatement(
reduceLeft(
currentModuleInfo.exportedNames!.slice(i, i + chunkSize),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

.slice() substitutes the length of the array if the second argument is greater than .length

@RyanCavanaugh RyanCavanaugh requested review from weswigham and rbuckton and removed request for weswigham June 9, 2020 16:49
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.

The input file for the test is missing~

@ericanderson
Copy link
Contributor Author

Sorry @weswigham, what do I need to provide besides tests/baselines/reference/manyConstExports.js

@weswigham
Copy link
Member

tests/cases/compiler/manyConstExports.ts

@typescript-bot typescript-bot added the For Milestone Bug PRs that fix a bug with a specific milestone label Aug 3, 2020
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.

@rbuckton so long as you're OK with this, I think this is an OK fix for the issue, for now.

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

@DanielRosenwasser we might want to ship this in the next 4.0 patch release. What do you think?

@sandersn sandersn merged commit 79f919e into microsoft:master Sep 4, 2020
tvh pushed a commit to factisresearch/TypeScript that referenced this pull request Oct 9, 2020
* Fixes stack overflow when exporting a lot in commonjs

Fixes microsoft#38691

* Add missing test files
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Milestone Bug PRs that fix a bug with a specific milestone
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Maximum call stack size exceeded when a single file has many exports
5 participants