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

[email protected] broken #173

Closed
kopax opened this issue Oct 10, 2019 · 15 comments
Closed

[email protected] broken #173

kopax opened this issue Oct 10, 2019 · 15 comments

Comments

@kopax
Copy link

kopax commented Oct 10, 2019

I have traced an error coming from react-styleguidist that use this plugin.

The [email protected] is the last working version, between 4.3.5 and 4.3.8 I got all my build broken with Maximum call stack size

Could we please fix terser while waiting they release a fix and publish a new version of this plugin with fixed [email protected]?

Thanks

@alexander-akait
Copy link
Member

They fixed other bugs (degradation create other bugs), better wait official release terser and as i written before use lock file in future 😄

@amcclain
Copy link

Curious - does this project need to specify the terser version out to the patch version so specifically?

We are also seeing issues with the latest versions of terser. We're pinning [email protected], but when resolving deps yarn will warn with Resolution field "[email protected]" is incompatible with requested version "terser@^4.3.8". We can ignore that and keep pushing onwards, but I am curious if the latest release of this plugin depends on features/changes in terser 4.3.8 (in which case we should really pin the version of this plugin back as well).

Hope that makes sense. Thanks for any additional info.

@alexander-akait
Copy link
Member

alexander-akait commented Oct 10, 2019

You can ignore this warning, we will do this because some developers don't update their lock file and use old npm/yarn version (Some of them have not been updated transitive deps before). I think it's time to not do this anymore. We have compatibility with terser@3 and terser@4 (any minor and any patch versions)

@kopax
Copy link
Author

kopax commented Oct 10, 2019

They fixed other bugs (degradation create other bugs), better wait official release terser and as i written before use lock file in future

I still believe lock file are not the solution here.

In fact, I have no lock file in all my modules and I was able to build one using --before=2019-09-01, so this is not the issue.

I have found terser was the issue thanks to the diff of two lock file.

They fixed other bugs (degradation create other bugs), better wait official release terser

If you just release a fix, and then re-release on official, then you will revert a change on a good library for a change on a bad library. I have checked and the change added can be fixed in a later release, but that doesn't really concern you.

All that concern you is do you want to support the % of people not able to use 4.3.4 or do you rather welcome people with > 4.3.5 and exclude old users?

Please release a short fix so we don't get all our work degraded. (we can't release http documentation because of that)

@alexander-akait
Copy link
Member

If you just release a fix, and then re-release on official, then you will revert a change on a good library for a change on a bad library. I have checked and the change added can be fixed in a later release, but that doesn't really concern you.

What do you mean? Terser fix some problems in latest version but get new bugs, revert to 4.3.4 return old bugs but avoid new, so there is no perfect solution (in both cases you can potentially get bugs). I'd rather wait a bit new terser patch release than do revert.

If you use lock file you you might not have to worry about it at all because you would use the old version of terser plugin and terser himself and catch problem on CI/manual updating.

All that concern you is do you want to support the % of people not able to use 4.3.4 or do you rather welcome people with > 4.3.5 and exclude old users?

I can't understand.

Please release a short fix so we don't get all our work degraded. (we can't release http documentation because of that)

I think it’s better to wait for tomorrow and only then do release with revert because reverting to 4.3.4 return other bug for other developers.

Don’t forget that this is open source software and the fastest way is try to fix a problem and send a PR. Some developers do a lot for others but do not get anything in return and it would be great if others would sometimes help.

@kopax
Copy link
Author

kopax commented Oct 10, 2019

Again lock file is not the issue here, I can have a lock file in my project and all the subproject using it will fail. Lockfile are for application. Using lockfile while a bug like this will still be happening for my users since lockfile only guarantee the project, it does not guarantee when install in other app, this is what is happening here accross all my projects.

Also as I said, If you need to generate a lock file at a particular date to reproduce an old install you can still use --before=$date but this will break if you update some modules with some release after $date.

I can't upgrade dependencies normally and maintain my package.

I meant that 4.3.4 is working for the most and 4.3.5 is supposed to fix things for the few.
If everytime a fix goes wrong and package owner refuse to revert fix then we have an issue using semver. At first 4.3.5 should have mot been release and 4.3.4 should have been tested.

I understand why you prefere to wait tomorrow and see if the owner of terser manifest his self, but if he don't it would be nice to fix your package since 4.3.5 is a major issue to us as it impact all our projects. I can take care of the PR.

haimich added a commit to haimich/copyundpasta that referenced this issue Oct 13, 2019
@amcclain
Copy link

You can ignore this warning, we will do this because some developers don't update their lock file and use old npm/yarn version (Some of them have not been updated transitive deps before). I think it's time to not do this anymore. We have compatibility with terser@3 and terser@4 (any minor and any patch versions)

Thanks for the reply. Understood we can ignore the warning, but it gets flagged in CI, we get questions on it, etc. But we're happily ignoring it now 😄.

That said, I agree when you say "it's time to not do this anymore" - you could let the downstream developers be responsible for taking updates into their lock files (or not) as works for them, and only specify the minimum versions that are required to work with the plugin and not trigger any bugs that you know will bounce back at you!

Much appreciated for the work on the plugin all around. 👍

@kopax
Copy link
Author

kopax commented Oct 14, 2019

I agree. But that said, I am still stuck and a small action could be taken to solve this for end users in my situation. If styleguidist/react-styleguidist#1443 get passed then we react-styleguidist and terser user can go around that problem. I still thing that a fixed version is apporpriate, and this is what I generally do in all my repo of bootstrap-styled because a lot of people break and does not respect semver and on a stack of modules, it does delay.

@alexander-akait
Copy link
Member

/cc @fabiosantoscode friendly ping, bug fixed?

@kopax
Copy link
Author

kopax commented Oct 15, 2019

@fabiosantoscode
Copy link
Contributor

I will be reverting what is probably the culprit commit today. I've been trying to reproduce this with a minimal code sample, but since it's webpack (I suppose) being called with react-styleguidist without access to changing options it's really hard to reproduce.

@fabiosantoscode
Copy link
Contributor

fabiosantoscode commented Oct 15, 2019

Just so you have an idea, I've replaced node_modules/terser with a symbolic link to my development terser, and patched the minify function such that it doesn't minify at all. However, the output is still minfied :/

@fabiosantoscode
Copy link
Contributor

Oh, my bad. I hit a cache.

@fabiosantoscode
Copy link
Contributor

New Terser version should be fixed now.

@kopax
Copy link
Author

kopax commented Oct 16, 2019

it's true, thanks for fixing this.

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

No branches or pull requests

4 participants