-
-
Notifications
You must be signed in to change notification settings - Fork 180
Webpack's removes classnames when minifying/uglifying ES6 code with inheritance #269
Comments
@anatoly314 Thanks for issue, Yes looks like a bug. Need investigate who output this in not right way. |
I do not think that this is an error in webpack or uglify. The syntax itself is so far correct because on the one hand you can have anonymous classes and these can also extend from other classes. And because the class name is not referenced anywhere in the file it can be optimized away. So I do not see any problem in that part of the code: n.exports = class extends r { The error itself happens in this part of the code, which is afaik not part of webpack, but a module provided by @anatoly314: class Parent {
constructor() {
if (this.constructor.name === 'Parent'){
throw new TypeError("Parent class is abstract - cant be instance");
}
}
} A test like Because of that, the code of @anatoly314 should look like this: class Parent {
constructor() {
if (this.constructor === Parent){
throw new TypeError("Parent class is abstract - cant be instance");
}
}
} And then the code would not have any problem with this kind of minification. |
@tndev we don't change any code, just run uglify on code which go from |
@evilebottnawi Yes and the resulting code generated is perfectly fine, there is no syntax error or invalid minification. The error happens in the code of |
@tndev, your solution doesn't work. When you run minified code under NodeJS v6.9.1,
But when we run it from NodeJS v8.9.1 it returns:
In both case I can't know from which class an object was instantiated. |
@anatoly314 looks as won't fix in |
@evilebottnawi The displayed names are wrong that is true, but comparison is still correct (at leat here with 6, 8 and 9). The output of: console.log(this.constructor)
console.log(this.constructor.name, (this.constructor === Parent)) Is for
And for
Or do you get And Is for
And for
So imho is it just a display bug of V8. |
@tndev, it's mind-blowing but it works. But why, or it's just one of evil parts of JavaScript? |
@anatoly314 writing |
Thank you very much, you helped us a lot and today I've learned a bit more about JavaScript. |
Why is this issue closed? Using
God this doesn't make any sense. How would I know if my module will be minified or not? Why would I have to write code that assume how it will be consumed? This is nonsense. If this is a perfectly valid ES6 syntax then webpack should not break it. This is a bug. |
The point of minification is that is it changes the names of variables, functions and classes. Reflection features are considered to be not reliable in all languages if it comes to minification and obfuscation as of that. They already take care for parts where it is not avoidable or checkable:
But should this be extended to a part where a reliable alternative exists? So the question is more why can't (or don't want to) use
As you can see |
|
@tndev But what if you need to actually get the constructor name? Not for comparison. Just for the sake of using it for output. @evilebottnawi Actually, I don't explicitely used uglifyjs. I just launch |
@evilebottnawi I assume this is a webpack 4 issue. Why they are minifying by default and thus not supporting ES6 is beyond me. |
@ericmorand It is actually not webpack related. That's how uglify/terser do the minification. The minifiyer will convert this:
to:
But that's not webpacks fault, nor something webpack needs to care about. Those are related uglifyjs2 issues:
But those reflection features should really only be used for debugging/logging or code completion purpose. |
But an uglifier should not assume anything on the way the code is written as long as its respects the language specifications. Which is the case there. A source code is only validated against the language specifications. The source code should not make any asumption on how it will be consumed; the consumers should not make any asumption on how the code is written. This...
...and this...
...are different algorithms. The first one is perfectly valid and pass the unit tests. The second one doesn't. So uglifyjs2 is actually making lossy changes to the code, and thus it's a bug. Except if someone can point me to the uglifyjs2 specifications that say that uglifyjs2 is lossy and will break the original code expected result. Imagine a compiler or a transpiler that would make the source code not giving the expected result even though it is perfectly valid and pass the unit tests. This would clearly be considered as a bug. And this is exactly what is happening here. |
it's not just an uglifyjs2 issue the same problem (with Twing) applies when babel-minify is used instead of uglifyjs2. you need to switch of mangleing to have Twing working:
@ericmorand how much work would it be to solve it in Twing? |
@corneliusweiss, not much, fortunately, just in one place. I will have it fixed in no time. My rant is more conceptual - a transpiler should not break a perfectly valid code. |
Then again, since we have It might make sense to add an option that disables this feature, but name mangling is extremely useful as a minification tool for those who don't use constructor names for anything. |
But it doesn't only cause issues with constructor names. Named arguments also suffer from this. Twing can't be used with mangling because named arguments are part of Twig specifications. Once again, a transpiler should not break a valid code. The bundle size argument is not meaningful. |
If you need to check the marks at replace steps in all TextNodes, you can use recursive functions and check if its a Node or a TextNode with constructor.name, but using uglify-js (case of vue.js) it breaks the code, according to this thread (webpack-contrib/uglifyjs-webpack-plugin#269) a better idea should be use elem.constructor === Parent instead of element.construcot.name === 'Parent'. For this purpose you can import Node and TextNode and check.
…104 -Updating js minifier to terser because uglify started to remove class name, probably due to some dependant module change, and that broke our event handling logic: we used to use full class names to bind to events but now those classes names are minified to 'e' or 'r' etc. Probably issue: webpack-contrib/uglifyjs-webpack-plugin#269
…237 -Updating js minifier to terser because uglify started to remove class name, probably due to some dependant module change, and that broke our event handling logic: we used to use full class names to bind to events but now those classes names are minified to 'e' or 'r' etc. Probably issue: webpack-contrib/uglifyjs-webpack-plugin#269
Updating js minifier to terser because uglify removes class name, probably due to some dependant module change, and that broke our event handling logic: we used to use full class names to bind to events but now those classes names are minified to 'e' or 'r' etc. Probably issue: webpack-contrib/uglifyjs-webpack-plugin#269
This is a copy of my answer on StackOverflow, I suspect that it's a bug so I decided to open an issue here too.
Project to demonstrate the issue: https://github.com/anatoly314/webpack-uglify-inheritence
Webpack's removes classnames when minifying/uglifying ES6 code with inheritance:
There's MVCE code which we try to minify/uglify:
Class Child:
index.js which invokes
Child
class:Module Parent inside
node_modules
:The whole output I'll post to the end of the question, here I want to post only relevant lines which I think cause a wrong behavior (lines 33-37 from original output):
Why a classname is missing here:
class extends r
? I expect that value will be mangled but will exist, can I consider it as a bug? I tried to usekeep_classnames
flag but it keeps original class names which unacceptable.We're using:
Our
webpack.config.js
:The whole minified/uglified code from the example above:
The text was updated successfully, but these errors were encountered: