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): parallel execution #1341

Merged
merged 9 commits into from Dec 5, 2022
Merged

feat(terser): parallel execution #1341

merged 9 commits into from Dec 5, 2022

Conversation

tada5hi
Copy link
Member

@tada5hi tada5hi commented Nov 10, 2022

Rollup Plugin Name: terser

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.

List any relevant issue numbers:
1334

Description

Execute minify process in parallel

@shellscape
Copy link
Collaborator

Thanks for opening this one. Is this based on existing work somewhere or brand new work? If existing, please document from where so we have a reference point. If it's new, please add some code comments for code in spots you think people may have questions or that may raise eyebrows.

@tada5hi
Copy link
Member Author

tada5hi commented Nov 10, 2022

Thanks for opening this one. Is this based on existing work somewhere or brand new work? If existing, please document from where so we have a reference point. If it's new, please add some code comments for code in spots you think people may have questions or that may raise eyebrows.

The idea of merging and populating the nameCache of a sub process is from @TrySound, but it is only the idea not the code. Nevertheless, i already mentioned him in the Readme.
The rest is completly concepted and implemented by myself.

@shellscape
Copy link
Collaborator

That was less about attribution and more about helping the other maintainers understand the change.

@tada5hi
Copy link
Member Author

tada5hi commented Nov 10, 2022

That was less about attribution and more about helping the other maintainers understand the change.

No problem 😊
If you or anyone else have any questions, I'm more than happy to help ✌️ 😇

packages/terser/src/module.ts Outdated Show resolved Hide resolved
@michaelfaith
Copy link

michaelfaith commented Nov 10, 2022

That was less about attribution and more about helping the other maintainers understand the change.

No problem 😊 If you or anyone else have any questions, I'm more than happy to help ✌️ 😇

I think the request was for more documentation / comment blocks.

@tada5hi
Copy link
Member Author

tada5hi commented Nov 21, 2022

@shellscape can you check if everything is fine now?
If something is still missing or should be enhanced, let me know.
Greetings
Peter

packages/terser/src/index.ts Outdated Show resolved Hide resolved
@shellscape
Copy link
Collaborator

@TrySound please have a look through this one. once merged, it will probably be a good idea to deprecate the other plugin (https://www.npmjs.com/package/rollup-plugin-terser) and archive the old repo, pointing people here. If you need a hand with that, please feel free to add me to each and I'll take care of it.

packages/terser/README.md Outdated Show resolved Hide resolved
@tada5hi
Copy link
Member Author

tada5hi commented Nov 28, 2022

@shellscape is TrySound's feedback required or how long do you plan to wait for his review?

What's weird is that I'm not listed as a contributor for the initial work on the plugin.
https://github.com/rollup/plugins/graphs/contributors
Is this because of the strategy you used to merge my commits?

@shellscape
Copy link
Collaborator

It may be that Github doesn't update that in realtime. You're also on the merge commit. 2d67e34. I wouldn't get too wrapped up with vanity metrics.

You should however add yourself to https://github.com/rollup/plugins/blob/master/CODEOWNERS for this plugin so you get automated notifications when someone opens a PR against it.

is TrySound's feedback required or how long do you plan to wait for his review?

I would like it, yes. he's a busy guy so it may take some time. moving forward that won't be the case.

@shellscape
Copy link
Collaborator

Welp. It's been a week so we'll move forward. Thanks for your patience.

@shellscape shellscape merged commit 140e06b into rollup:master Dec 5, 2022
@jakearchibald
Copy link

This caused a regression by using __filename in a module #1366.

@tada5hi
Copy link
Member Author

tada5hi commented Dec 6, 2022

It may be that Github doesn't update that in realtime. You're also on the merge commit. 2d67e34. I wouldn't get too wrapped up with vanity metrics.

You should however add yourself to https://github.com/rollup/plugins/blob/master/CODEOWNERS for this plugin so you get automated notifications when someone opens a PR against it.

is TrySound's feedback required or how long do you plan to wait for his review?

I would like it, yes. he's a busy guy so it may take some time. moving forward that won't be the case.

@shellscape no problem. I was just wondering 😅
Good point. I added myself to the list in the recent pull request.

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.

None yet

5 participants