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

fix(minify): upgrade terser to v5, etc. #20

Conversation

Francois-Esquire
Copy link

@Francois-Esquire Francois-Esquire commented Jan 24, 2023

This PR upgrades terser to v5 and adds a test case to test for ES2020 syntax (specifically optional chaining). Relates to issue #17 and #19.

@brodybits
Copy link
Owner

brodybits commented Jan 24, 2023

adds a test case to test for ES2020 syntax

It would be awesome if you want to put this info into the commit message.

Unfortunately #18 (remove Node.js pre-14 support) seems to be a prerequisite for this. I may do this soon.

I may have a few more nits coming on this, and some more comments coming on original issue #17.


P.S. Adding a link to related issue: TrySound#61

@brodybits brodybits changed the title fix(minify): upgrade terser to v5 fix(minify): upgrade terser to v5, etc. Jan 24, 2023
Copy link
Owner

@brodybits brodybits left a comment

Choose a reason for hiding this comment

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

Looks good in general.

My one major criticism is the use of "next" as ES2020 would be 2-3 years old by now.

IMHO adding tests is not as important when updating dependencies. I think tests are way more important when making bug fixes or adding new features.

It looks to me like this proposal includes several related items, which is why I added ", etc." to the title of this PR:

  • update Terser to v5
  • add test of ES2020
  • test with updated @rollup/plugin-terser
  • update README.md with updated @rollup/plugin-terser

README.md Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
tests/fixtures/next-features.js Outdated Show resolved Hide resolved
tests/index.test.js Outdated Show resolved Hide resolved
tests/index.test.js Outdated Show resolved Hide resolved
@brodybits
Copy link
Owner

@Francois-Esquire I have just released another patch and started version 0.14.0 with minimum Node.js version 14. I would like to get the terser v5 upgrade out pretty soon. If you can rebase this and address my comments soon then I can merge it in. Otherwise I may just do this upgrade without the test case.

I will start updating some other dependencies now.

@brodybits brodybits self-assigned this Jan 24, 2023
@brodybits brodybits marked this pull request as draft January 24, 2023 20:30
@brodybits
Copy link
Owner

@Francois-Esquire never mind, I am taking the liberty to fix this up myself and will merge this as a squash commit. I would like to get this finished soon. Thanks and apologies.

@brodybits brodybits marked this pull request as ready for review January 24, 2023 20:44
@brodybits brodybits merged commit 541d3f8 into brodybits:main Jan 24, 2023
@Francois-Esquire
Copy link
Author

@brodybits Thanks so much for taking the lead on this, truly appreciate it! And thanks for staying committed to this package, has been a huge help for us 🙌

@brodybits
Copy link
Owner

These updates are now published in v0.14.1 (0.14.0 was published with missing contents).

FYI I am currently available for short-term and long-term opportunities in case anyone in your organization may be interested, private contact chris.brody@gmail.com.

@Francois-Esquire Francois-Esquire deleted the fix/support-latest-syntax-when-minifying branch January 25, 2023 18:57
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

2 participants