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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

update terser to latest version (5) #46

Closed
alexander-akait opened this issue Aug 3, 2020 · 39 comments 路 Fixed by #63
Closed

update terser to latest version (5) #46

alexander-akait opened this issue Aug 3, 2020 · 39 comments 路 Fixed by #63
Assignees
Labels
help wanted Extra attention is needed webpack5
Projects

Comments

@alexander-akait
Copy link
Collaborator

alexander-akait commented Aug 3, 2020

Will be great to do it ASAP to avoid multiple dependencies, thanks for work on the package, plus update old deps too 馃槃 If you need I can help at the end of week

@DanielRuf
Copy link
Contributor

to avoid multiple dependencies

Not sure what you mean with this.

plus update old deps too

Which dependencies exactly do you mean?

According to https://github.com/terser/terser/blob/master/CHANGELOG.md#v500-beta0 the minimum required Node version is 10(but the code in package.json still says 6). And there are a few more breaking changes. So this will not happen asap.

This needs further clarification because webpack 4 supports Node 6+.
https://github.com/DanielRuf/html-minifier-terser/blob/master/package.json#L52

cc @fabiosantoscode

@alexander-akait
Copy link
Collaborator Author

@DanielRuf You can do major release, as we will do for terser-webpack-plugin. developers who want to support node@6, can you old version, this is a normal technical process, drop old node and update terser is not hard

@DanielRuf
Copy link
Contributor

DanielRuf commented Aug 4, 2020

I will wait for the feedback of @fabiosantoscode. I do not plan to maintain two major versions at the same time so the upgrade will not happen soon.

Please keep in mind that dependents like the html-webpack-plugin would use the old version which I would not maintain then.

https://www.npmjs.com/package/html-minifier-terser

developers who want to support node@6, can you old version, this is a normal technical process, drop old node and update terser is not hard

I can not simply drop that because webpack 4 will still support Node 6 for a very long time. So this is not an option. Otherwise there will be issues like #22 and more which produces extra work and makes the upgrade and major version bump senseless for the near future.

@DanielRuf
Copy link
Contributor

I'll wait until webpack 5 is released and webpack 4 deprecated.

@alexander-akait
Copy link
Collaborator Author

alexander-akait commented Aug 4, 2020

I will wait for the feedback of @fabiosantoscode. I do nopt plan to maintain two major versions at the same time so the upgrade will not happen soon.

Okay, I will do fork and will use it in webpack packages. And you should not maintain two packages, you need keep support the latest version and keep it up to date

What you are trying to do only creates difficulties and problems:

  • Almost of developers migrate on node@10
  • Using old version of terser do not have bug fixes, so you can break code
  • Using it with latest webpack will create duplicate packages, because we will do release terser-webpack-plugin with 5 version today
  • No new features and fixes from terser
  • Keeping old verison of deps and support node@6 is potential unsafe, because many old deps other have security problems

@alexander-akait
Copy link
Collaborator Author

alexander-akait commented Aug 4, 2020

I'll wait until webpack 5 is released and webpack 4 deprecated.

No need, it only creates problems, most of developers do not use node 6, almost loaders and plugins supports only node@10, you can look at package.json

@DanielRuf
Copy link
Contributor

Not sure why this is needed and I don't think this is helpful. This will likely break all webpack 4 setups.

Still, what do you want to resolve by upgrading to terser 5 in this package here?

@DanielRuf
Copy link
Contributor

No need, it only creates problems, most of developers do not use node 6, almost loaders and plugins supports only node@10, you can look at package.json

Which package.json? I look at the package.json of webpack. And The html-webpack-plugin maintained by @jantimon.

@DanielRuf
Copy link
Contributor

No need, it only creates problems

I see no problem by supporting minimum Node version. Please clarify which problems you mean.
Currently the whole discussion does not provide good reasons to upgrade right now or rush this until webpack 5 is finally released.

@alexander-akait
Copy link
Collaborator Author

I update original answer #46 (comment)

@DanielRuf
Copy link
Contributor

And you should not maintain two packages

This is not what I mean, if we upgrade (which results in html-minifier-terser v6) we may still have to support v5 by maintaining a second branch (which is done by many big projects).

@alexander-akait
Copy link
Collaborator Author

alexander-akait commented Aug 4, 2020

Big projects can should migrate on new node version, supporting EOL version is unsafe

@alexander-akait
Copy link
Collaborator Author

Still, what do you want to resolve by upgrading to terser 5 in this package here?

This package uses in html-loader, so we need to upgrade terser to avoid future problems with minimizing

@alexander-akait
Copy link
Collaborator Author

Currently the whole discussion does not provide good reasons to upgrade right now or rush this until webpack 5 is finally released.

Are you seriously? Node 6 does not receive security updates, it potentially contains vulnerabilities, no project will use this in production

@DanielRuf
Copy link
Contributor

DanielRuf commented Aug 4, 2020

To clarify my opinion:

What you are trying to do only creates difficulties and problems:

  • Almost of developers migrate on node@10

Which problems / difficulties does this cause? I see none.

  • Using old version of terser do not have bug fixes, so you can break code

I am still waiting for the feedback from @fabiosantoscode.

  • Keeping old verison of deps and support node@6 is potential unsafe, because many old deps other have security problems

You did not show any relevant advisories or npm audit reports ;-)

Using old version of terser do not have bug fixes, so you can break code

This can be easily backported.

Big projects can should migrate on new node version, supporting EOL version is unsafe

Well, tell that webpack and others ;-)

Are you seriously? Node 6 does not receive security updates, it potentially contains vulnerabilities, no project will use this in production

Well, tell that webpack and others ;-)

This package uses in html-loader, so we need to upgrade terser to avoid future problems with minimizing

Citation needed, where are currently problems with minimizing? I see no relevant bugfixes at https://github.com/terser/terser/blob/master/CHANGELOG.md#v500

Please calm down and do not rush. At least producing pressure on me does not help at all. And I want to avoid issues like in the past due to breaking installs reported to @jantimon and others.

@alexander-akait
Copy link
Collaborator Author

You did not show any relevant advisories or npm audit reports ;-)

Please read node docs about minimum usage, they recommend never to use EOL node version (it is insecurity), safety is always matter

This can be easily backported.

terser does not bug port fixes in old version

Well, tell that webpack and others ;-)

Already, so we drop node@6 and node@8 support from all loaders and plugins, I tell you how core developers of webpack

Citation needed, where are currently problems with minimizing? I see no relevant bugfixes at https://github.com/terser/terser/blob/master/CHANGELOG.md#v500

in operator now taken into account during property mangle.
Fixed infinite loop in face of a reference loop in some situations.

What is it? It is fixes, so if somebody will use it in code, it will be problem.

Please calm down and do not rush. At least producing pressure on me does not help at all. And I want to avoid issues like in the past due to breaking installs reported to @jantimon and others.

Okay, as I said before time to fork this projects and maybe time to fork html-webpack-plugin, lately this creates only problems, my attempt to fix the process leads to nowhere, requests to fix and pay attention to very important things are ignored. I'm not putting pressure on you, I'm just trying to explain that such an approach currently creates problems, and will create even more in the future, I'm trying to prevent them at the start. I don't see any problem with dependency updates and don't use a old node. Supporting EOL node version is the worst idea. webpack@4 support node@6 due historical reasons. The new webpack version might even skip version 10 to avoid the same problem in future. All this I say as a developer who spends most of the time maintaining webpack and webpack ecosystem.

@alexander-akait
Copy link
Collaborator Author

And the most important thing - backporting is the last resort when there is no way to update at all, it is a very bad idea to use this as a standard approach

@DanielRuf
Copy link
Contributor

Good luck with this. terser 5 is only 3 days old. I see no good reasons to rush this.

@alexander-akait
Copy link
Collaborator Author

And please tell me what this package has to do with the webpack? What if I use it in completely different places and ask to do update, because my code is broken due terser bugs? Still don't think this is a problem? i can create repository with issue or will that be ignored too?

@fabiosantoscode
Copy link

Hey there! I missed my mentions :)

Terser 5 does drop older node support. Thanks for pointing out that package.json still says node 6 :) I'll change it when I have the time.

I don't think there's any rush in moving to Terser 5 either! It's just been released.

The real issue with updating is probably not going to be the node version (as most of your users are probably on node >=10), but the fact that the minify() function is now async (I'm very sorry but I have to update to the new source-map). This will warrant a major bump on this package as well.

Error handling changed (errors are thrown instead of returned) but that shouldn't be a big issue IMO.

@DanielRuf
Copy link
Contributor

DanielRuf commented Aug 18, 2020

The real issue with updating is probably not going to be the node version (as most of your users are probably on node >=10), but the fact that the minify() function is now async (I'm very sorry but I have to update to the new source-map). This will warrant a major bump on this package as well.

Error handling changed (errors are thrown instead of returned) but that shouldn't be a big issue IMO.

Ok, it seems we may have to change a few things then and ensure that the code still works as expected. I'll plan some PR for September or October and a major release for the end of this year or next year when probably webpack 5 is released and I can discuss and coordinate this with the other package maintainers so they are aware of the planned roadmap and coming changes.

@alexander-akait
Copy link
Collaborator Author

probably webpack 5 is released and I can discuss and coordinate this with the other package maintainers so they are aware of the planned roadmap and coming changes

You may not have noticed, but I am maintainer and I am asking you to do this asap, but you again ignore me

@alexander-akait
Copy link
Collaborator Author

But no need, fork is WIP and webpack will be migrate on this

@DanielRuf
Copy link
Contributor

DanielRuf commented Aug 18, 2020

You may not have noticed, but I am maintainer and I am asking you to do this asap, but you again ignore me

I am aware of that but I see no good reasons to rush here (only for webpack) =)

Reopening as this is still relevant, not just for webpack. Nothing critical that has to be done now.

@DanielRuf DanielRuf reopened this Aug 18, 2020
@terser terser locked as off-topic and limited conversation to collaborators Aug 18, 2020
@DanielRuf DanielRuf self-assigned this Aug 18, 2020
@DanielRuf DanielRuf added this to To do in development via automation Aug 18, 2020
@terser terser unlocked this conversation Aug 22, 2020
@abdonrd
Copy link

abdonrd commented Sep 11, 2020

There are any news here about update to Terser 5? Thanks in advance!

@DanielRuf
Copy link
Contributor

No news here so far.

@fabiosantoscode
Copy link

Ok, it seems we may have to change a few things then and ensure that the code still works as expected.

@DanielRuf fortunately await and Promise.resolve() turn non-promise things into promises.

PS I've migrated the terser org to travis-ci.com to fix an issue in PR builds over at the terser repo, hope you don't mind!

@laurentpayot
Copy link
Contributor

I found out that my JS code inside my HTML was not minified at all because I was using the now widespread optional chaining operator ?..
Optional chaining operator is supported by Terser since version 5.2.0, but html-minifier-terser only uses version 4.6.3.

It's been a while since Terser 5 release, maybe it's time to update Terser to the latest version so people can use this package with modern JS syntax 馃槈

@DanielRuf DanielRuf added the help wanted Extra attention is needed label Oct 20, 2020
@DanielRuf
Copy link
Contributor

DanielRuf commented Oct 20, 2020

PRs are very welcome. I have currently no time.

@fabiosantoscode
Copy link

Please be patient fellow humans. Updating to Terser 5 is not a matter of bumping a version in package.json.

@slorber
Copy link

slorber commented Oct 20, 2020

Hi.

As the Docusaurus 2 maintainer, we would also benefit from the terser v5 upgrade.
How complex is that upgrade? Can an external contributor easily work on this?

We have weird Yarn behavior (only affecting monorepos), where some users end up running this html-minifier-terser against Terser v5 instead of v4, which make the build fail. This seems to be the case when some other package of the monorepo depends on file-loader. (facebook/docusaurus#3515)

(undefined) TypeError: Cannot read property 'replace' of undefined
    at Object.options.minifyJS (main:18614:28)

My workaround to ensure Terser V4 keep being used is to use nohoist:

"workspaces": {
    "packages": [
      "docs",
      "other-project"
    ],
    "nohoist": [
      "**/html-minifier-terser",
    ]
  }, 

We already have a few affected users by this problem.

This issue is likely not the fault of this package, but was just curious if others are sharing the same motivations for this terser v5 upgrade, and maybe my workaround will be useful to others in the same situation?

@DanielRuf
Copy link
Contributor

We have weird Yarn behavior (only affecting monorepos), where some users end up running this html-minifier-terser against Terser v5 instead of v4, which make the build fail

Even with yarn resolutions?

@slorber
Copy link

slorber commented Oct 20, 2020

As far as I remember, I tried yarn resolutions and it didn't work. Wasn't able to get it fixed without nohoist.

If you are curious there are repros to try to fix :)
Like this one where yarn install && cd docs && yarn build won't pass: https://github.com/taylorreece/docusaurus-minify-troubles

@fabiosantoscode
Copy link

I suspect this has to be fixed at the yarn level.

Asking for a version of a package and not getting something outside that range is a very important, very critical task of any package manager.

I suspect that if you have source-map installed then you have two incompatible versions of it installed? The latest one is async (which is why Terser is async)

@slorber
Copy link

slorber commented Oct 21, 2020

Yes I agree @fabiosantoscode , it's likely a Yarn issue (or maybe related to node resolution too).
I debugged the issue and can clearly see that this package calls Terser 5, and fail on resultPromise.replace() (terser 5) instead of the expected result.replace() (terser 4).

@alexander-akait
Copy link
Collaborator Author

I'll wait until webpack 5 is released and webpack 4 deprecated.

@DanielRuf do we need wait something

webpack@5 was released a long time ago

@DanielRuf
Copy link
Contributor

DanielRuf commented Feb 3, 2021

I'll wait until webpack 5 is released and webpack 4 deprecated.

@DanielRuf do we need wait something

webpack@5 was released a long time ago

Feel free to create the fork (or provide a PR), I do not have much time at the moment and in the next weeks.

@alexander-akait
Copy link
Collaborator Author

@DanielRuf we need to change our API on async, do you agree? Don't wan to waste time

@DanielRuf
Copy link
Contributor

I agree with this.

@sibiraj-s sibiraj-s mentioned this issue Jul 7, 2021
development automation moved this from To do to Done Aug 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed webpack5
Projects
development
  
Done
Development

Successfully merging a pull request may close this issue.

6 participants