Skip to content

Commit

Permalink
[BUGFIX release] Fix uglification introduced bug with super wrapping.
Browse files Browse the repository at this point in the history
We check to see if a function contains `_super` / `.call` / `.apply`
before attempting to super wrap a given method (this saves quite a bit
of extra super wrapping for functions that do not need it).
Unfortunately, a number of platforms that we support do not support
calling `func.toString()` so we attempt to detect this and fall back
to always super wrap everything mode.

We have roughly this code (from
[here](https://github.com/emberjs/ember.js/blob/b4718218dbe5ffe7736c485a594248b20977c621/packages/ember-metal/lib/utils.js#L257-L271))
to detect if we can call `toString()` and get the original source:

```javascript
  let sourceAvailable = (function() {
    return this;
  }).toString().indexOf('return this;') > -1;
```

This works perfectly for development builds, but unfortunately not
when minified.  Take a look at the minified source and you will see why:

```javascript
var e=function(){return this}.toString().indexOf("return this;")>-1;
```

Note that minifier has stripped the trailing semicolon from the function
body we are attempting to check against, but sadly our `indexOf` check
still contains the semicolon!

**tldr;** We super wrap every function in uglified builds.

---

This commit removes the trailing semicolon from the source check, and
adds a test that should hopefully allow us to detect if this is
accidentally reintroduced.
  • Loading branch information
rwjblue committed Oct 8, 2015
1 parent 1f85853 commit 7894cae
Show file tree
Hide file tree
Showing 3 changed files with 15 additions and 3 deletions.
2 changes: 2 additions & 0 deletions packages/ember-metal/lib/environment.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ if (hasDOM) {
hasDOM: true,
isChrome: !!window.chrome && !window.opera,
isFirefox: typeof InstallTrigger !== 'undefined',
isPhantom: !!window.callPhantom,
location: window.location,
history: window.history,
userAgent: window.navigator.userAgent,
Expand All @@ -36,6 +37,7 @@ if (hasDOM) {
hasDOM: false,
isChrome: false,
isFirefox: false,
isPhantom: false,
location: null,
history: null,
userAgent: 'Lynx (textmode)',
Expand Down
4 changes: 2 additions & 2 deletions packages/ember-metal/lib/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -254,10 +254,10 @@ export function guidFor(obj) {

const HAS_SUPER_PATTERN = /\.(_super|call\(this|apply\(this)/;

const checkHasSuper = (function () {
export const checkHasSuper = (function () {
let sourceAvailable = (function() {
return this;
}).toString().indexOf('return this;') > -1;
}).toString().indexOf('return this') > -1;

if (sourceAvailable) {
return function checkHasSuper(func) {
Expand Down
12 changes: 11 additions & 1 deletion packages/ember-metal/tests/utils_test.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { inspect } from 'ember-metal/utils';
import { inspect, checkHasSuper } from 'ember-metal/utils';
import environment from 'ember-metal/environment';

QUnit.module('Ember Metal Utils');

Expand All @@ -14,3 +15,12 @@ QUnit.test('inspect outputs the toString() representation of Symbols', function(
expect(0);
}
});

// Only run this test on browsers that we are certain should have function
// source available. This allows the test suite to continue to pass on other
// platforms that correctly (for them) fall back to the "always wrap" code.
if (environment.isPhantom || environment.isChrome || environment.isFirefox) {
QUnit.test('does not super wrap needlessly [GH #12462]', function(assert) {
assert.notOk(checkHasSuper(function() {}), 'empty function does not have super');
});
}

0 comments on commit 7894cae

Please sign in to comment.