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

[BUGFIX release] Fix uglification introduced bug with super wrapping. #12463

Merged
merged 1 commit into from
Oct 9, 2015

Conversation

rwjblue
Copy link
Member

@rwjblue rwjblue commented Oct 8, 2015

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) to detect if we can call toString() and get the original source:

  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:

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.

Fixes #12462.

@stefanpenner
Copy link
Member

Lgtm

@ef4
Copy link
Contributor

ef4 commented Oct 8, 2015

LGTM.

The test will fail on platforms that don't support Function.toString, but that is kinda the point of the test: detecting unnecessary super wrapping, which those platforms unfortunately do have.

@rwjblue
Copy link
Member Author

rwjblue commented Oct 8, 2015

We could add a phantomjs detection to https://github.com/emberjs/ember.js/blob/master/packages/ember-metal/lib/environment.js#L27-L28, and wrap the test in a if (env.isPhantom || env.isChrome || env.isFirefox). This allows the test suite to pass on other browers that may or may not need the "always wrap" fallback path, but ensures we continue to pass on browsers that we know should support function() { }.toString().

Thoughts?

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.
@rwjblue rwjblue force-pushed the do-not-super-wrap-everything branch from 7938a56 to 7894cae Compare October 8, 2015 23:14
@rwjblue
Copy link
Member Author

rwjblue commented Oct 8, 2015

Added the phantomjs check I mentioned above, the test suite should now continue to pass even on platforms that do not support function source.

@ef4
Copy link
Contributor

ef4 commented Oct 8, 2015

👍

rwjblue added a commit that referenced this pull request Oct 9, 2015
[BUGFIX release] Fix uglification introduced bug with super wrapping.
@rwjblue rwjblue merged commit e15e5d5 into emberjs:master Oct 9, 2015
@rwjblue rwjblue deleted the do-not-super-wrap-everything branch October 9, 2015 02:10
@dschmidt
Copy link
Contributor

dschmidt commented Oct 9, 2015

Awesome, thanks!

@stefanpenner
Copy link
Member

this is likely going to be a nice little perf post, since i suppose we where super wrapping many more things before.

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

Successfully merging this pull request may close these issues.

4 participants