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.

Sign up for GitHub

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

Jump to bottom

feat(terser): initial release #1323

Merged
merged 6 commits into from
Oct 27, 2022
Merged

feat(terser): initial release #1323

merged 6 commits into from
Oct 27, 2022

Conversation

tada5hi
Copy link
Member

@tada5hi tada5hi commented Oct 14, 2022 •
edited
Loading

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:

Description

@tada5hi tada5hi mentioned this pull request Oct 14, 2022
Closed
2 tasks
@dasa dasa mentioned this pull request Oct 15, 2022
Closed
@tada5hi
Copy link
Member Author

tada5hi commented Oct 19, 2022

@shellscape something missing ?

@shellscape
Copy link
Collaborator

shellscape commented Oct 19, 2022 •
edited
Loading

@tada5hi huh?

@TrySound please weigh in here.

relevant comment from previous PR #1182 (comment)

of note, we'll only add a new plugin here if it has a dedicated maintainer as we're very short on help in this repo.

@tada5hi tada5hi changed the title Add terser plguin feat: Add terser plguin Oct 19, 2022
@tada5hi
Copy link
Member Author

tada5hi commented Oct 19, 2022 •
edited
Loading

If @TrySound will not update his own terser plugin for rollup, like it looks like at the moment, @shellscape i will maintain the terser plugin in this monorepo.

But I would really like to use terser with rollup v3+ 🙈😅

Greetings
Peter

@shellscape
Copy link
Collaborator

Thanks for volunteering to maintain it. As long as @TrySound is cool with this, we can proceed. We try to respect the authors of established plugins as much as is possible. I'll ping him on some other channels to see if I can get his eyes on this.

@shellscape shellscape changed the title feat: Add terser plguin feat(terser): initial release Oct 20, 2022
@shellscape
Copy link
Collaborator

Followup: I have an older conversation from May 4, 2022 with @TrySound archived in which he stated that he was OK with his plugin being imported here. Let's give him a day or so to reply and if he doesn't have the chance, we can proceed.

In the mean time, please look into the errors with CI which will need to be resolved before we can merge: https://github.com/rollup/plugins/actions/runs/3291680331/jobs/5426137184#step:8:21

@TrySound
Copy link
Member

Looks like this plugin is missing workers support. Do you plan to add it in other PR?

@tada5hi
Copy link
Member Author

tada5hi commented Oct 21, 2022 •
edited
Loading

Thanks for volunteering to maintain it. As long as @TrySound is cool with this, we can proceed. We try to respect the authors of established plugins as much as is possible. I'll ping him on some other channels to see if I can get his eyes on this.

Very gladly 😊 . I totally agree and I'm open to anything. Like i already mentioned, it would be nice to use terser with rollup v3+ however this should be possible😅

@shellscape i resolved the issues

@tada5hi
Copy link
Member Author

tada5hi commented Oct 21, 2022

Looks like this plugin is missing workers support. Do you plan to add it in other PR?

I'm completely open If there is a need, I am very happy to integrate it again.

@willeastcott willeastcott mentioned this pull request Oct 21, 2022
Open
@tada5hi
Copy link
Member Author

tada5hi commented Oct 22, 2022

@shellscape can you permit workflow execution again, to check if everything is fine now ?

@dasa dasa mentioned this pull request Oct 22, 2022
Closed
4 tasks
@michaeldzjap michaeldzjap mentioned this pull request Oct 23, 2022
Closed
@jcscottiii jcscottiii mentioned this pull request Oct 24, 2022
Closed
@shellscape
Copy link
Collaborator

@tada5hi looks like a linting error. from the repo root, run pnpm -w fix:js

@tada5hi
Copy link
Member Author

tada5hi commented Oct 25, 2022 •
edited
Loading

@tada5hi looks like a linting error. from the repo root, run pnpm -w fix:js

@shellscape thx. I unfortunately forgot to lint the root readme 🙈
It also looks like all tests have now been successful

@tada5hi
Copy link
Member Author

tada5hi commented Oct 26, 2022

@shellscape is there something missing i should do ?

This was referenced Oct 27, 2022
Open
Closed
@shellscape shellscape merged commit 2d67e34 into rollup:master Oct 27, 2022
@shellscape
Copy link
Collaborator

thanks!

shellscape pushed a commit that referenced this pull request Oct 27, 2022
@tada5hi @shellscape
feat(terser): initial release (#1323)
047fc1d 
* build(terser): add terser plugin to collection

* build(terser): add README.md notice

* fix(terser): test-suite + rollup plugin definition

* style(terser): applied prettier rules

* style(terser): apply prettier on root README.md + minor eslint fixes

* test(terser): removed snapshot test due incorrect path with ava
@limonte
Copy link

limonte commented Oct 27, 2022

Great work @tada5hi ❤️

Tested the newly released plugin on https://github.com/sweetalert2/sweetalert2 and everything works perfectly 🎉

@dasa
Copy link
Contributor

dasa commented Oct 31, 2022

I'm completely open If there is a need, I am very happy to integrate it again.

@tada5hi that'd be great (and thanks for adding this plugin). We're seeing about a four minute slowdown using this plugin vs. rollup-plugin-terser.

@michaelfaith
Copy link
Contributor

michaelfaith commented Oct 31, 2022 •
edited
Loading

I'm completely open If there is a need, I am very happy to integrate it again.

@tada5hi that'd be great (and thanks for adding this plugin). We're seeing about a four minute slowdown using this plugin vs. rollup-plugin-terser.

Perhaps it would be good to open as an issue, since this was merged? Hope to see the update, as well.

@dasa dasa mentioned this pull request Nov 1, 2022
Closed
@dasa
Copy link
Contributor

dasa commented Nov 1, 2022

Perhaps it would be good to open as an issue, since this was merged?

Issue #1334

@tada5hi
Copy link
Member Author

tada5hi commented Nov 5, 2022

I'm completely open If there is a need, I am very happy to integrate it again.

@tada5hi that'd be great (and thanks for adding this plugin). We're seeing about a four minute slowdown using this plugin vs. rollup-plugin-terser.

i will try to implement this soon 😊

@tada5hi
Copy link
Member Author

tada5hi commented Nov 5, 2022

Very glad, that you guys like and appreciate the contribution 😊 ✌️

stof
stof reviewed Nov 10, 2022
View reviewed changes
packages/terser/README.md

## Requirements

This plugin requires an [LTS](https://github.com/nodejs/Release) Node version (v14.0.0+) and Rollup v1.20.0+.
Copy link

Choose a reason for hiding this comment

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

it actually requires Rollup 2+ according to the peer dependency

Copy link
Member Author

Choose a reason for hiding this comment

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

good point 🙈

@GrosSacASac
Copy link

Could not find info, is this a drop in replacement for rollup-plugin-terser ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lukastaegert lukastaegert lukastaegert left review comments

@dasa dasa dasa left review comments

@stof stof stof left review comments

@TrySound TrySound TrySound approved these changes

@shellscape shellscape Awaiting requested review from shellscape shellscape is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@tada5hi @shellscape @TrySound @limonte @dasa @michaelfaith @GrosSacASac @stof @lukastaegert