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

optimize: true breaks es6 compatibility #117

Closed
MattMcFarland opened this issue Jan 16, 2017 · 8 comments
Closed

optimize: true breaks es6 compatibility #117

MattMcFarland opened this issue Jan 16, 2017 · 8 comments

Comments

@MattMcFarland
Copy link

MattMcFarland commented Jan 16, 2017

Steps to reproduce:

  1. Create a blank project using bankai

  2. For your client.js code, simply add the following lines:

var foo = `bar`
var func = (args) => console.log

Expected results
Should be no error

Actual results
the node instance blows up and appears that uglifyjs is parsing with es5 rules.

some notes/findings:
Using babelify, es2020 do not work

When trying to use those as transforms in package.json or passed into browserify using the js property you get another error stating that the path to client.js cannot be found. If you disable yoyoify you then get the same damn uglifyjs error anyway. I think that the transforms might not be called in the correct order, but I am not sure.

yoyoify transpiles backticks if you use them with bel/yo/choo - Optimize will not fail if you are using template tag strings that are passed in to bel/yo/choo - but that is the only case. using fat arrow functions though will not work at all.

Suggested fix
Let developers overload js or optimize and have the ability to explicitly use different transforms when optimize is true. this could be messy but this would add some flexibility.

Or maybe ditch the uglifyjs transform all together and just pipe the finished browserify stream to uglifyjs when everything is done?? idk if that will work but right now I'm just not sure how to get this to work other than to use es5 only which makes me sad.

See also: mishoo/UglifyJS#1411

@MattMcFarland
Copy link
Author

I am happy to send PR but I dont know what strategy we should take for fixing this.. thoughts are welcome :)

@yoshuawuyts
Copy link
Member

yoshuawuyts commented Jan 18, 2017 via email

@MattMcFarland
Copy link
Author

MattMcFarland commented Jan 18, 2017

Both fail, when disabling one or the other uglifyjs does not like template tags nor does it like arrow functions.

@MattMcFarland
Copy link
Author

I think one way to fix this would be to transpile before it is sent to uglifyjs. However, if you transpile first then call yoyoify it blows up.

The problem is that yoyoify has to be called before you transpile and you have to transpile before you run uglifyjs - I havent confirmed this but that is the only thing that makes logical sense to me atm.

@yoshuawuyts
Copy link
Member

yoshuawuyts commented Jan 22, 2017 via email

@rahatarmanahmed
Copy link

The workaround for this, as @yoshuawuyts says, is to have duplicate yoyoify and sheetify transforms before babelify. Not ideal but it seems to work.

const jsOpts = {
  js: {
    transform: [
      'envify',
      'sheetify/transform',
      'yo-yoify',
      ['babelify', { presets: ['latest'] }]
    ]
  },
  optimize: true
}
const clientPath = path.join(__dirname, '../client/app.js')
client = bankai(clientPath, jsOpts)

@delvedor
Copy link

delvedor commented Apr 2, 2017

Hi guys, same problem here!
Solved in this way:

"script": {
"build:browser": "bankai build browser/index.js --optimize --electron build-browser/"
}
"browserify": {
  "transform": [
    "unassertify",
    "yo-yoify",
    "es2020"
  ]
}

@yoshuawuyts
Copy link
Member

You can now do --uglify false and it should no longer crash, can just pull in your own uglify (: Given the lack of activity I think this issue has been resolved. Feel free to reopen if it hasn't been. Cheers!

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