Skip to content

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

Merged
bluwy merged 12 commits into
vitejs:mainfrom
nguyenquangsang160596:feature/add-maxWorkers-for-terserOptions
Sep 28, 2023
Merged

feat(terser): add maxWorkers option for terserOptions#13858
bluwy merged 12 commits into
vitejs:mainfrom
nguyenquangsang160596:feature/add-maxWorkers-for-terserOptions

Conversation

@sangquangnguyen
Copy link
Copy Markdown
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.

@bolt-new-by-stackblitz
Copy link
Copy Markdown

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
Copy Markdown
Contributor Author

sangquangnguyen commented Jul 15, 2023

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

@sangquangnguyen
Copy link
Copy Markdown
Contributor Author

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

@sangquangnguyen
Copy link
Copy Markdown
Contributor Author

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

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

patak-cat 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
Copy Markdown
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
Copy Markdown
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-cat patak-cat 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-cat patak-cat 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
Copy Markdown
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
@sangquangnguyen
Copy link
Copy Markdown
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