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

Add reasonable uglify-js options. #7077

Merged
merged 1 commit into from
Jun 1, 2017

Conversation

rwjblue
Copy link
Member

@rwjblue rwjblue commented Jun 1, 2017

These options should always have been specified, and are good defaults.

I could have sworn that these defaults were already present, but I could not find them in this repo, in ember-cli-uglify repo, or the broccoli-uglify-sourcemap repo...

@krisselden
Copy link
Contributor

@rwjblue should we not deep merge?

@rwjblue
Copy link
Member Author

rwjblue commented Jun 1, 2017

@krisselden - We do merge deeply via lodash.defaultsDeep here.

@stefanpenner
Copy link
Contributor

@rwjblue [BUGFIX release] ?

@rwjblue
Copy link
Member Author

rwjblue commented Jun 1, 2017

This targets beta at the moment. I'm fine with release also, can update the PR target in the morning.

@stefanpenner
Copy link
Contributor

This targets beta at the moment. I'm fine with release also, can update the PR target in the morning.

seems likely good to get this out asap IMHO.

These options should always have been specified, and are
good defaults.
@rwjblue rwjblue changed the base branch from beta to release June 1, 2017 12:46
@rwjblue
Copy link
Member Author

rwjblue commented Jun 1, 2017

retargeted and rebased

@stefanpenner
Copy link
Contributor

:shipit:

@rwjblue rwjblue merged commit 9c22e94 into ember-cli:release Jun 1, 2017
@rwjblue rwjblue deleted the uglify-js-options branch June 1, 2017 14:39
@cibernox
Copy link
Contributor

Out of curiosity, how does this affect performance?

@stefanpenner
Copy link
Contributor

@cibernox what performance are you curious about build/runtime?

@cibernox
Copy link
Contributor

cibernox commented Jun 25, 2017

Precisely. This is tagged as performance but it's not clear to me to which one it refers. I assume it refers to some sort IIFE optimization, and I'm curious to read more about that.

@stefanpenner
Copy link
Contributor

stefanpenner commented Jun 25, 2017

@cibernox TL;DR

uglify stripped the leading ( here define('asdf',[], (function() { } )), preventing https://www.npmjs.com/package/ember-cli-optimize from working. more details here

@krisselden
Copy link
Contributor

@cibernox negate_iife hurts the performance at runtime of parsing IIFE to save one character. Turning statements into sequences, like a series of define() statements is also costly at parsing, but @rwjblue would have needed to set it to sequences: false or 0

kevinansfield added a commit to kevinansfield/Ghost-Admin that referenced this pull request Sep 18, 2017
refs TryGhost/Ghost#8815, TryGhost/Ghost#8840, TryGhost/Ghost#8842, TryGhost/Ghost#8849
- `ember-cli` 2.14 introduced some [new defaults for uglify-js](ember-cli/ember-cli#7077) that resulted in CloudFlare's Auto Minify feature mangling the JS files and causing syntax errors
- revert the `semicolons: false` option to restore compatibility

**Note:** This does _not_ mean it's recommended to use CloudFlare's Auto Minify feature. It's still recommended that all CloudFlare's performance settings are [disabled for /ghost* URLs](https://docs.ghost.org/docs/troubleshooting#section-ghost-admin-not-loading)
ErisDS pushed a commit to TryGhost/Admin that referenced this pull request Sep 18, 2017
refs TryGhost/Ghost#8815, TryGhost/Ghost#8840, TryGhost/Ghost#8842, TryGhost/Ghost#8849
- `ember-cli` 2.14 introduced some [new defaults for uglify-js](ember-cli/ember-cli#7077) that resulted in CloudFlare's Auto Minify feature mangling the JS files and causing syntax errors
- revert the `semicolons: false` option to restore compatibility

**Note:** This does _not_ mean it's recommended to use CloudFlare's Auto Minify feature. It's still recommended that all CloudFlare's performance settings are [disabled for /ghost* URLs](https://docs.ghost.org/docs/troubleshooting#section-ghost-admin-not-loading)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants