-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Assets Precompile fails #306
Comments
Still facing this after #291 |
Have you re-run webpacker:install and replaced outdated configs? |
@gauravtiwari as I mentioned I ran both the command again and I overwrote all the files. Any idea why this might be happening? For now is there a way to disable |
Ahh strange! Sure just remove uglify from production.js for now. I will test again, it's seems like certain syntax isn't supported or perhaps requires a babel plugin 🙂 |
If possible could you please share your vue app code or equivalent example so I can test it out at my end. |
Sure, I'll share my code. |
That error occurs because uglifier-js doesn't support shorthand method syntax, i.e. You need to lock uglifier in See: https://github.com/lautis/uglifier#uglifier and mishoo/UglifyJS#448 for more details Edit: It seems from that issue thread that the community is moving towards babel minify (babili). We can perhaps consider that as the default compressor instead. |
@RavenXce That's correct, but since this is compiled through babel it shouldn't be a problem. I think it needs required babel plugin to compile that syntax - https://babeljs.io/docs/plugins/transform-function-bind/ |
@gauravtiwari I believe the issue is that the So the core issue is that the compressor must still support at least what browsers are currently supporting at the end of the day. I believe uglifyjs may not be up to this task. |
Sorry I'm on mobile so I can't edit my above comment atm but I just wanted to add that I ran into this particular issue too, and the correct plugin to use is: https://babeljs.io/docs/plugins/transform-es2015-shorthand-properties/. Edit: This babel option is also relevant to this issue, and gives the same alternative recommendations - https://github.com/babel/babel-preset-env#targetsuglify |
I know that not everyone will consider this a proper solution but:
and in
Solves the issue described here. |
Just use this for now, will get this updated soon 👍 {
"presets": [
["env", {
"modules": false,
"targets": {
"node": "current"
},
"useBuiltIns": true
}],
"react"
]
} |
@gauravtiwari depending on what we want, I think there are three options we can look at to make things less confusing for users. Right now, almost everyone will run into this issue as the chosen default babel preset doesn't match what uglify supports. 1. Target uglify:
This should work, but will basically cause babel to always transpile to ES5, which also means features which browsers already support will still be polyfilled regardless of what the user wants to target. May not be ideal as a default because of that. 2. Use uglify2 + harmony branch:
This allows uglify to support most ES6 syntax. It is what I'm currently doing at the moment. However, it may not be ideal as a default too, as there are no actual releases that support ES6 (have to use git url to target the branch in package.json). 3. Switch uglify to babli:I have not tried this, but seems to be the current go-to for minifying JS? Is meant to support ES6 as well. |
Alright, closing this for now 👍 Lets use what we have for now and revisit later once of the options above are usable. |
I'm using option 1 as suggested by @RavenXce, but I also had to install and use |
@flybayer Are you using |
@gauravtiwari, yes, I am. |
And you still have to use |
I know :/ Yes, I still have to use babel-polyfill {
"presets": [
["env", {
"modules": false,
"targets": {
"node": "current",
"uglify": true
},
"useBuiltIns": true
}]
],
"plugins": [
"transform-object-rest-spread"
],
} |
Hmm strange. |
I will file an issue on the preset repo 👍 |
@flybayer you need to use babel-polyfill because you are setting Don't think it has anything to do with this issue @gauravtiwari |
@RavenXce Right, but that's what
The ugilify option states this, if I am reading it correctly:
|
@gauravtiwari I think you're misinterpreting the instructions. It just means that It is clarified later in the documentation:
|
@RavenXce Oh yes, that's I agree that we need |
It's just that we don't need to explicitly declare the imports right? |
Users will still need to declare the Actually I'm not sure why We should simply show how to enable them in the |
@RavenXce think |
Yup, that's correct. |
I think I am missing something, I updated my gems and since then this is failing.
I also ran
Here is my trace:
The text was updated successfully, but these errors were encountered: