prefix for mangle property names #1475

dabretin · 2017-02-08T15:15:32Z

It could be very useful to set profix of mangle names.
It could permit to restrict access (at runtime) to "private" properties/methods on proxified objects.

Sorry for the double post...
I have previously done an error (no default value for prefix in mangle_properties)

dabretin · 2017-02-08T15:17:16Z

Previous post #1472

kzc · 2017-02-08T15:22:40Z

~~Could you please close #1472 if this PR replaces it?~~

To consider this PR we would need bin/uglifyjs and minify() tests in test/mocha/, as well as tests in test/compress/.

kzc · 2017-02-08T15:27:17Z

Could you please remove the [Harmony] prefix in the title if you intend this to be merged to the branch master?

kzc · 2017-02-08T16:03:30Z

test/mocha/minify.js

+		it("Should add prefix \"__\" for mangle properties (not quoted)", function()
+		{
+            var js = 'a["foo"] = "bar"; a.color = "red"; x = {"bar": 10};';
+            var result = Uglify.minify(js, {


There's a formatting issue. Please use spaces instead of tabs.

kzc · 2017-02-08T16:05:41Z

Also would need documentation in README.md

kzc · 2017-02-08T16:09:59Z

Could you please change the bin/uglifyjs flag name to mangle-props-prefix?

kzc · 2017-02-08T16:11:15Z

test/compress/mangle-prefix.js

+    }
+}
+
+


Please only have a single empty line between tests.

kzc · 2017-02-08T16:14:16Z

test/mocha/minify.js

@@ -58,6 +58,46 @@ describe("minify", function() {
            assert.strictEqual(result.code,
                    'a["foo"]="bar",a.a="red",x={"bar":10};');
        });
+
+        it("Should add prefix \"__\" for mangle properties (not quoted)", function() {


Please remove \"__\" from description.

kzc · 2017-02-08T16:14:25Z

test/mocha/minify.js

+                    'a["foo"]="bar",a.__a="red",x={"bar":10};');
+        });
+
+        it("Should add prefix \"__\" for mangle properties (with quoted)", function() {


Please remove \"__\" from description.

kzc · 2017-02-08T16:17:48Z

bin/uglifyjs

@@ -74,6 +74,7 @@ You need to pass an argument to this option to specify the name that your module
    .describe("reserve-domprops", "Make (most?) DOM properties reserved for --mangle-props")
    .describe("mangle-props", "Mangle property names (0 - disabled, 1 - mangle all properties, 2 - mangle unquoted properies)")
    .describe("mangle-regex", "Only mangle property names matching the regex")
+    .describe("mangle-prefix", "Prefix of the mangle properties")


Please rename this flag to mangle-props-prefix in bin/uglifyjs. The property name in lib/propmangle.js is fine.

kzc · 2017-02-08T16:21:06Z

test/mocha/minify.js

+                }
+            });
+            assert.strictEqual(result.code,
+                    'a["foo"]="bar",a.__a="red",x={"bar":10};');


please collapse lines 78 and 79 into one line

kzc · 2017-02-08T16:21:54Z

test/mocha/minify.js

+                }
+            });
+            assert.strictEqual(result.code,
+                    'a["__a"]="bar",a.__b="red",x={"__c":10};');


please collapse into one line

kzc · 2017-02-08T16:32:42Z

@dabretin I apologize for not raising this issue earlier - I'm afraid I don't understand the use case of this PR:

#1472 (comment)

Why would anyone want to add a prefix to the mangled property name? That just wastes space in the minified output.

dabretin · 2017-02-08T16:41:31Z

It can enable to restrict access at runtime to properties.
After mangle, it's not possible to detect the "private" orientation of a property. The only possibility seems to be a prefix of the obfuscated properties...

kzc · 2017-02-08T17:04:57Z

README.md

@@ -150,6 +150,7 @@ The available options are:
                                notation. You can override these by setting
                                them explicitly on the command line.
  --mangle-regex                Only mangle property names matching the regex
+  --mangle-props-prefix         Prefix of the mangle properties


more apt description: "Prefix added to the property name after mangling."

kzc · 2017-02-08T17:11:17Z

Need a command line test in test/mocha/ to test the new bin/uglifyjs flag. See test/mocha/cli.js for an example.

kzc · 2017-02-08T20:12:22Z

bin/uglifyjs

@@ -74,6 +74,7 @@ You need to pass an argument to this option to specify the name that your module
    .describe("reserve-domprops", "Make (most?) DOM properties reserved for --mangle-props")
    .describe("mangle-props", "Mangle property names (0 - disabled, 1 - mangle all properties, 2 - mangle unquoted properies)")
    .describe("mangle-regex", "Only mangle property names matching the regex")
+    .describe("mangle-props-prefix", "Prefix of the mangle properties")


Please make this description match the README.

kzc · 2017-02-08T20:28:30Z

This PR will not be approved without a command line test for bin/uglifyjs.

example: https://github.com/mishoo/UglifyJS2/blob/1eaa211e0932105439d98d4f03a981f157f0a77c/test/mocha/cli.js#L93-L102

kzc · 2017-02-08T21:34:21Z

test/input/mangle-prefix/sample.js

@@ -0,0 +1,28 @@
+var Test = function()
+{
+  this.someVar = 123;


Please use kick brace style throughout this file:

var Test = function() {

kzc · 2017-02-08T21:34:41Z

test/input/mangle-prefix/sample.js

+  this.someVar = 123;
+};
+
+


Just use a single empty line between functions.

kzc · 2017-02-08T21:40:40Z

test/input/mangle-prefix/sample.js

+  this.someVar = 123;
+};
+
+


Please use a single empty line between functions throughout the file.

kzc · 2017-02-09T02:12:44Z

This PR appears to be low risk and now has sufficient tests and documentation.

Please squash into a single commit and assign an appropriate commit comment.

Thanks.

dabretin · 2017-02-09T08:52:24Z

I'm not confortable with git so sorry in advance for my (probably stupid) questions...
Is the last comment for me ?
If it is...how can I do that ?

alexlamsl · 2017-02-09T08:59:49Z

@dabretin hope these help:
http://gitready.com/advanced/2009/02/10/squashing-commits-with-rebase.html
https://help.github.com/articles/about-git-rebase/

dabretin · 2017-02-09T13:42:47Z

I think it's done :)

kzc · 2017-02-09T20:00:23Z

If this PR gets in I think it could very well be the last new mangle option. They are getting harder to support and test with each new addition and combination.

dabretin mentioned this pull request Feb 8, 2017

[Harmony] - prefix for mangle property names #1472

Closed

dabretin changed the title ~~[Harmony] - prefix for mangle property names~~ prefix for mangle property names Feb 8, 2017

kzc reviewed Feb 8, 2017

View reviewed changes

test/compress/mangle-prefix.js Outdated

}

}

Copy link

Contributor

kzc Feb 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please only have a single empty line between tests.

kzc reviewed Feb 8, 2017

View reviewed changes

test/input/mangle-prefix/sample.js Outdated

this.someVar = 123;

};

Copy link

Contributor

kzc Feb 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just use a single empty line between functions.

kzc reviewed Feb 8, 2017

View reviewed changes

Add prefix to property name after mangling

96e5f90

dabretin force-pushed the master branch from 2693a3a to 96e5f90 Compare February 9, 2017 10:28

alexlamsl force-pushed the master branch from 3f5a7b7 to 7e3e9da Compare September 20, 2017 20:43

alexlamsl force-pushed the master branch from 45e1940 to ab36b9b Compare June 23, 2018 20:02

alexlamsl force-pushed the master branch 2 times, most recently from 7d1c0a7 to b468103 Compare July 5, 2018 10:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

prefix for mangle property names #1475

prefix for mangle property names #1475

dabretin commented Feb 8, 2017

dabretin commented Feb 8, 2017

kzc commented Feb 8, 2017 •

edited

Loading

kzc commented Feb 8, 2017

kzc Feb 8, 2017

dabretin Feb 8, 2017

kzc commented Feb 8, 2017

kzc commented Feb 8, 2017

kzc Feb 8, 2017

kzc Feb 8, 2017

kzc Feb 8, 2017

kzc Feb 8, 2017 •

edited

Loading

kzc Feb 8, 2017

kzc Feb 8, 2017

kzc commented Feb 8, 2017

dabretin commented Feb 8, 2017

kzc Feb 8, 2017

kzc commented Feb 8, 2017

kzc Feb 8, 2017

kzc commented Feb 8, 2017

kzc Feb 8, 2017

kzc Feb 8, 2017

kzc Feb 8, 2017

kzc commented Feb 9, 2017

dabretin commented Feb 9, 2017

alexlamsl commented Feb 9, 2017

dabretin commented Feb 9, 2017

kzc commented Feb 9, 2017

prefix for mangle property names #1475

Are you sure you want to change the base?

prefix for mangle property names #1475

Conversation

dabretin commented Feb 8, 2017

dabretin commented Feb 8, 2017

kzc commented Feb 8, 2017 • edited Loading

kzc commented Feb 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kzc commented Feb 8, 2017

kzc commented Feb 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kzc Feb 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kzc commented Feb 8, 2017

dabretin commented Feb 8, 2017

Choose a reason for hiding this comment

kzc commented Feb 8, 2017

Choose a reason for hiding this comment

kzc commented Feb 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kzc commented Feb 9, 2017

dabretin commented Feb 9, 2017

alexlamsl commented Feb 9, 2017

dabretin commented Feb 9, 2017

kzc commented Feb 9, 2017

kzc commented Feb 8, 2017 •

edited

Loading

kzc Feb 8, 2017 •

edited

Loading