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

feat(terser): add maxWorkers option for terserOptions #13858

Merged

Conversation

sangquangnguyen
Copy link
Contributor

@sangquangnguyen sangquangnguyen commented Jul 15, 2023

Description

Closes #13856

Additional context

Take a look at 2 repos
@rollup/plugin-terser supports maxWorkers
rollup-plugin-terser supports numWorkers


They are supporting maxWorkers property for customizing the worker when running terser minify.
I encountered 137 error (Out of Memory) in CI build because of that. So allow custom the worker pool is the best solution

Before After
Screenshot 2023-07-15 at 13 53 42 Screenshot 2023-07-16 at 11 57 06

What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the PR Title Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@stackblitz
Copy link

stackblitz bot commented Jul 15, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@sangquangnguyen sangquangnguyen changed the title Adding new config for defining number of workers [Build] Adding maxWorkers for terserOptions support as rollup-plugin-terser and @rollup/plugin-terser Jul 15, 2023
@sangquangnguyen sangquangnguyen changed the title [Build] Adding maxWorkers for terserOptions support as rollup-plugin-terser and @rollup/plugin-terser chore: Adding maxWorkers for terserOptions support as rollup-plugin-terser and @rollup/plugin-terser Jul 15, 2023
@sangquangnguyen sangquangnguyen changed the title chore: Adding maxWorkers for terserOptions support as rollup-plugin-terser and @rollup/plugin-terser chore: adding maxWorkers for terserOptions support as rollup-plugin-terser and @rollup/plugin-terser Jul 15, 2023
@sangquangnguyen
Copy link
Contributor Author

sangquangnguyen commented Jul 15, 2023

Please help me review this PR @sapphi-red , thank you ❤️

@sangquangnguyen
Copy link
Contributor Author

Hi @patak-dev , please help me review this issue and this PR , thank you 💟

@sangquangnguyen
Copy link
Contributor Author

Could you help me take a look at this PR please @bluwy ❤️ ?

@patak-dev patak-dev added the p2-nice-to-have Not breaking anything but nice to have (priority) label Jul 17, 2023
@patak-dev
Copy link
Member

patak-dev commented Jul 17, 2023

@sangquangnguyen thanks for the PR! Please be more patient next time, it is completely normal that a PR or issue will take a couple of days to be reviewed/triaged. Notifying maintainers only generates more notifications for everybody, and it may make them less willing to engage.

I think we should add this feature. And reusing terserOptions sounds like the best we can do here. I added it to the team's board so we can discuss it. This will also take some time so I advise you to use a patched version of Vite (check patch-package or pnpm patch).

@sangquangnguyen
Copy link
Contributor Author

@sangquangnguyen thanks for the PR! Please be more patient next time, it is completely normal that a PR or issue will take a couple of issues to be reviewed/triaged. Notifying maintainers only generates more notifications for everybody, and it may make them less willing to engage.

I think we should add this feature. And reusing terserOptions sounds like the best we can do here. I added it to the team's board so we can discuss it. This will also take some time so I advise you to use a patched version of Vite (check patch-package or pnpm patch).

Cool, I will be more patient next time, thank @patak-dev for your reply and also your great advice. Have a nice day ;)

@bluwy
Copy link
Member

bluwy commented Jul 18, 2023

I also think this is a nice feature to add. I wonder if we ever go forward with #13584, maybe we can have a single top level config to control the max threads instead.

@patak-dev patak-dev changed the title chore: adding maxWorkers for terserOptions support as rollup-plugin-terser and @rollup/plugin-terser feat: adding maxWorkers for terserOptions support as rollup-plugin-terser and @rollup/plugin-terser Jul 27, 2023
@patak-dev patak-dev added this to the 5.0 milestone Jul 27, 2023
@bluwy bluwy changed the title feat: adding maxWorkers for terserOptions support as rollup-plugin-terser and @rollup/plugin-terser feat(terset): add maxWorkers option for terserOptions Sep 22, 2023
@bluwy bluwy changed the title feat(terset): add maxWorkers option for terserOptions feat(terser): add maxWorkers option for terserOptions Sep 22, 2023
bluwy
bluwy previously approved these changes Sep 22, 2023
@bluwy
Copy link
Member

bluwy commented Sep 28, 2023

Merging as we already have approval from the last meeting.

@bluwy bluwy merged commit 884fc3d into vitejs:main Sep 28, 2023
9 checks passed
@sangquangnguyen
Copy link
Contributor Author

Thanks team, love 💓

@sangquangnguyen sangquangnguyen deleted the feature/add-maxWorkers-for-terserOptions branch December 12, 2023 04:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: build p2-nice-to-have Not breaking anything but nice to have (priority)
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[Build] Need to support maxWorkers as rollup-plugin-terser and @rollup/plugin-terser
3 participants