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.

Sign up for GitHub

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

Jump to bottom

Use window instead of this in closures to improve obfuscation. #2413

Open
ghost opened this issue Aug 21, 2012 · 7 comments · May be fixed by #2630
Open

Use window instead of this in closures to improve obfuscation. #2413

ghost opened this issue Aug 21, 2012 · 7 comments · May be fixed by #2630
Labels
enhancement
Milestone
1.6.0

Comments

@ghost
Copy link

ghost commented Aug 21, 2012 •
edited by arian

I don't understand why the global object is still referenced by using the keyword this inside the closures since it won't be obfuscated/minimized. As an example, the following...

(function(){

    this.MooTools = { /*! ... */ };

    var typeOf = this.typeOf = function(item){ /*! ... */ };

})();

... will be minimized as ...

(function(){this.MooTools={/*! ... */};var a=this.typeOf=function(b){/*! ... */};})();

... while the now following ...

!function(window){

    window.MooTools = { /*! ... */ };

    var typeOf = window.typeOf = function(item){ /*! ... */ };

}(this);

... will be minimized this way:

!function(a){a.MooTools={/*! ... */};var b=a.typeOf=function(c){/*! ... */};}(this);

(Note: Negating the self invoking function [unless there is no returning value needed] saves the parentheses.)

--- Want to back this issue? **[Post a bounty on it!](https://www.bountysource.com/issues/55865-use-window-instead-of-this-in-closures-to-improve-obfuscation?utm_campaign=plugin&utm_content=tracker%2F22067&utm_medium=issues&utm_source=github)** We accept bounties via [Bountysource](https://www.bountysource.com/?utm_campaign=plugin&utm_content=tracker%2F22067&utm_medium=issues&utm_source=github).
@ibolmo ibolmo modified the milestones: 1.5.1, 1.6 Mar 3, 2014
@SergioCrisostomo
Copy link
Member

Tested this idea on a branch in my repo and then minified it with grunt-uglify.
The resulting code passed the specs and looks like

!function(a){a.MooTools={version:"1.5.1-dev",build:"%build%"};var b=a.typeOf=function(a){if(null==a)return"null"; etc.....

The original minified file (before this change) was 83Kb, after the change was also 83Kb, just some bytes difference.

I don't say this is a bad idea but got the idea that code source is already well written and that the occurrences of the this word is low in our code.

Should I send a PR with this changes or should we close this issue?

@kentaromiura
Copy link
Member

I don't like the idea of using window since it's the global object only in browsers, since we provide a server build it will be weird, we should use global instead.

@arian
Copy link
Member

arian commented Jul 6, 2014

Also, gzip. It's not that big of a deal iiuc.

@arian arian closed this as completed Jul 6, 2014
@ibolmo
Copy link
Member

ibolmo commented Jul 7, 2014

Well if the work has been done than go ahead.

On Sunday, July 6, 2014, Arian Stolwijk notifications@github.com wrote:

Closed #2413 #2413.

—
Reply to this email directly or view it on GitHub
#2413 (comment).

@gonchuki
Copy link
Contributor

gonchuki commented Jul 7, 2014

I actually like the improved readability with explicitly stating what this points to, and as @kentaromiura says it should be using global to keep it generic.

@kentaromiura
Copy link
Member

One more advantage of using global is that is a step forward cjs, since by exporting to global it will work as a cjs pseudo module in https://github.com/mootools/wrapup for example.

@kentaromiura kentaromiura reopened this Jul 7, 2014
@ghost
Copy link
Author

ghost commented Jul 7, 2014

It was just my intention to state that the reference of this could also be passed as an argument into the closure. It was not about how to name the argument. window was just an obvious guess. I agree that global would make more sense since there is a server build as well.

@SergioCrisostomo SergioCrisostomo linked a pull request Jul 7, 2014 that will close this issue
Open
@SergioCrisostomo SergioCrisostomo modified the milestones: 1.5.2, 1.5.1 Jul 8, 2014
@ibolmo ibolmo modified the milestones: 1.6.0, 1.5.2 Nov 11, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
1.6.0
Development

Successfully merging a pull request may close this issue.

Change this & window reference to a global passed in closure SergioCrisostomo/mootools-core
5 participants
@gonchuki @ibolmo @kentaromiura @arian @SergioCrisostomo