Do you mean -rename-public-vars=true instead of false? False should preserve the original behavior.
-- Josh Tynjala Bowler Hat LLC <https://bowlerhat.dev> On Thu, Feb 20, 2020 at 1:44 AM Alex Harui <[email protected]> wrote: > This change is breaking some modules when -rename-public-vars=false. > Let's say the main app has some method "foo" that gets renamed. It might > accidentally get a minified name that coincides with a public var in the > module. For tour de flex, some API gets the minified name "UP". A module > has code that references Keyboard.UP. > > In Pashmina's app, some API gets a minified name like "vs" and the module > has a property "vs" (for a ViewStack). > > We try to allow renaming of APIs in modules for size savings. The list of > renames in the main app is output to a file and read into the compiler when > compiling the module so it starts with the same renaming map. > > One thought I had about solving this is to add another compiler option > that allows a list of other names to not rename. So for the "UP" scenario, > I would compile the main app with, say, -forbidden-minified-names=UP. > Folks can probably work around the problem by using Keyboard.UP in the > main app, but that bloats the main app. The compiler can already read a > file of rename maps, so we could just use that, but I think a simpler > command-line list will be more convenient. > > Thoughts? > -Alex > > On 2/5/20, 12:18 PM, "Josh Tynjala" <[email protected]> wrote: > > Yeah, I'll make sure that users can control whether renaming happens > or not. > > -- > Josh Tynjala > Bowler Hat LLC < > https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbowlerhat.dev&data=02%7C01%7Caharui%40adobe.com%7C8dcbb865725d4151564408d7aa7893c9%7Cfa7b1b5a7b34438794aed2c178decee1%7C0%7C0%7C637165307165113089&sdata=a5evE514IT0GdVvL7w5IbOCcnyb%2FCwCqgxhOIlJsFvM%3D&reserved=0 > > > > > On Wed, Feb 5, 2020 at 11:51 AM Alex Harui <[email protected]> > wrote: > > > Great work, Josh! > > > > I have to say that the objectProperty output was adding pain to > debugging > > so looking forward to that going away. I'm assuming there will be > compiler > > options/directives to control renaming? I think there are some > scenarios > > where it is safe to have public variables renamed. > > > > Thanks, > > -Alex > > > > On 2/5/20, 11:44 AM, "Josh Tynjala" <[email protected]> > wrote: > > > > Thank you for the tips, Alex. Much appreciated. With your help, > I've > > determined how to use Closure compiler's Java API to prevent the > > renaming > > of a specific public variable that has not been @export-ed. Now, > I > > should > > be able to expand this prototype to a full version that prevents > the > > renaming of all public variables. > > > > -- > > Josh Tynjala > > Bowler Hat LLC < > > > https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbowlerhat.dev&data=02%7C01%7Caharui%40adobe.com%7C8dcbb865725d4151564408d7aa7893c9%7Cfa7b1b5a7b34438794aed2c178decee1%7C0%7C0%7C637165307165113089&sdata=a5evE514IT0GdVvL7w5IbOCcnyb%2FCwCqgxhOIlJsFvM%3D&reserved=0 > > > > > > > > > On Sun, Jan 19, 2020 at 4:58 PM Alex Harui > <[email protected]> > > wrote: > > > > > In response to your prior post, the reason I am saying it > removes > > control > > > is because I didn't see any option to not have the compiler > output > > > goog.reflect.objectProperty and I'm not clear everyone will > > want/need it. > > > > > > Regarding how to control Closure Compiler's renaming, the > details > > might be > > > changing because I believe I saw that Google refactored the > > renamer. At a > > > high-level, you probably know most of this, but for other folks > > reading, > > > the Closure Compiler is a set of Java Classes that form a > series of > > > Compiler Passes. Each Pass takes information (sometimes > source, > > sometimes > > > the AST, sometimes other information, and modifies the AST. > IIRC, a > > final > > > pass generates the output. There might be more than one pass > for > > output. > > > > > > The renaming pass we currently use can output as well as > accept a > > file of > > > rename mappings. I’m confident we can subclass or modify and > > replace the > > > renaming pass and feed it a set of mappings. If you look in > the > > > royale-compiler source, we've already done this for some other > > passes. > > > Look through the Closure compiler source for what happens to > the > > compiler > > > options: > > > > > > --variable_map_input_file > > > --property_map_input_file > > > > > > You can build examples/mxroyale/TourDeFlexModules which > outputs these > > > files to see what is in them. > > > > > > > > > We should also see if we can agree on the scenarios and > likelihood of > > > property access "by name". I can quickly think of: > > > > > > A) MXML setting properties by reference (high usage) > > > B) MXML setting properties by value (high usage) > > > C) Serializers/Deserializers (moderate usage) > > > D) [] bracket access by Literal (occasional usage) > > > E) [] bracket access by String Variable (occasional usage) > > > F) [] bracket access by Expression (infrequent usage) > > > > > > Exports can solve A. The compiler can easily detect D & E and > > prevent > > > renaming. For C, we "could" autogenerate exports for any > classes > > with > > > [RemoteClass] metadata or autogenerate getter/setters. > > > > > > To me, the only difficult case is F and it will rarely happen. > > Maybe we > > > can detect those and warn on that. > > > > > > Of course, I could be wrong... > > > -Alex > > > > > > > > > On 1/17/20, 10:08 AM, "Josh Tynjala" < > [email protected]> > > wrote: > > > > > > Comments inline. > > > > > > On Thursday, January 16, 2020, Alex Harui > > <[email protected]> > > > wrote: > > > > Maybe we should start by agreeing on facts and then > goals and > > then > > > discuss solutions. > > > > > > Yes, I think that's a good place to start. > > > > > > > > > > > Here are some facts that come to mind, not a complete > list. > > > > > > > > 1) An export does not prevent renaming. It builds an > alias. > > All > > > references within the set of sources to be minified are > renamed. > > > > > > Agreed. > > > > > > > 2) Closure's export mechanism only works on non-scalars > > (Object, > > > Arrays, > > > Functions) and not Number, String, Boolean because > non-scalars > > are > > > pass-by-reference instead of pass-by-value > > > > > > Agreed. > > > > > > > 3) The Closure Compiler is open source and designed to be > > extended > > > > > > Agreed. > > > > > > > 4) Use of goog.reflect.objectProperty is not necessarily > the > > only > > > way to > > > control renaming. It is the way recommended by Google for > those > > who > > > can't > > > extend the compiler. We are not constrained to modify our > output > > > because > > > we have control over the compiler. > > > > > > Could you share some details how we might have more > control over > > > Closure > > > compiler's renaming? It sounds like you know, at least > somewhat, > > how > > > to use > > > its lower-level Java APIs, but you've never shared the > details > > when > > > you've > > > mentioned them in this thread or in the past. > > > > > > I should add that I've personally tried to research this > topic > > myself, > > > but > > > I had a very hard time finding any information that wasn't > just > > someone > > > explaining to a JS developer that they needed to modify > their JS > > code. > > > I > > > eventually couldn't justify spending more time to keep > looking. > > > > > > > 5) The compiler knows things about how properties were > > accessed. > > > That > > > information is lost in the output in many cases. > Therefore, it > > should > > > be > > > better to inform the Google minifier directly from the > Royale > > compiler, > > > instead of leaving hints in the output. > > > > > > Agreed. I'm personally not fully convinced that the Royale > > compiler has > > > enough information for dynamic stuff (like for > serialization > > with type > > > Object), but that may be due to ignorance about Closure > > compiler's > > > capabilities. Even without knowing how it works, I can > imagine > > how it > > > might > > > be relatively easy to prevent renaming of public > variables, but > > the > > > dynamic > > > stuff is trickier. For the dynamic stuff, maybe it's just a > > matter of > > > Closure detecting when a variable is typed as Object, and > then > > it can > > > switch to ["string"] syntax on its own (instead of us > doing it > > in the > > > debug > > > build, like with -js-dynamic-access-unknown-members). > > > > > > > 7) We are pretty close to allowing renaming across > modules. > > It was > > > working for a while, but a scenario popped up that isn't > > currently > > > handled. We can pre-load the Closure renamer with a name > map. > > > > > > I haven't looked in detail at the module implementation and > > don't plan > > > to, > > > but I understand it well enough at a high level to say > "agreed" > > here > > > too > > > > > > > > > > > These are hypotheses, and not proven facts. > > > > 8) The big gain from not exporting everything is in dead > code > > removal > > > instead of shorter variable names > > > > > > Agreed, personally. It seems like others have expressed > interest > > in > > > both, > > > though. I hope that they'll be willing to prioriitze dead > code > > removal, > > > since it will probably have the bigger impact (my own tests > > removing > > > @export have been promising in this regard). > > > > > > > 9) Renaming can complicate and slow > > serialization/deserialization > > > > > > Agreed, and this is the harder portion to get working, I > think. > > > > > > However, if release builds didn't rename public variables, > and > > also > > > didn't > > > rename dynamic accesses, that would remove my biggest > > frustration with > > > how > > > ActionScript works in Royale/JS compared to SWF. If both > kept > > their > > > original names, things that feel broken today would "just > work" > > again. > > > > > > > > > > > IMO, we want to be heading in the direction of A) > allowing > > control > > > over > > > what gets renamed > > > > > > Agreed, but as I said before, I think that dead code > removal > > will have > > > more > > > impact than control over renaming, so if it's one or the > other, > > I'm > > > okay > > > with no control over renaming. > > > > > > > B) capturing information from the compiler, > > > > C) controlling the set of renames and exports directly, > not > > through > > > the > > > output. > > > > > > Agreed, being able to pass information Closure compiler on > the > > Java > > > side > > > would be better. than through the JS output > > > > > > > > > > > > > > My 2 cents, > > > > -Alex > > > > > > > > > > > > On 1/16/20, 2:48 PM, "Josh Tynjala" < > [email protected] > > > > > > wrote: > > > > > > > > Some additional context, if anyone is interested. > > > > > > > > At the request of Harbs, I am currently > investigating how > > we > > > might > > > remove > > > > @export from our generated JS code to improve the > > minimization > > > even > > > more. > > > > When I modified the compiler to skip emitting > @export in > > some > > > places, > > > a > > > > release build of TourDeJewel was initially broken. > When I > > added > > > > goog.reflect.objectProperty(), not only did it fix > setting > > public > > > variables > > > > in MXML, it also made that release build of > TourDeJewel > > start > > > working > > > again. > > > > > > > > -- > > > > Josh Tynjala > > > > Bowler Hat LLC < > > > > > > > > > https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbowlerhat.dev&data=02%7C01%7Caharui%40adobe.com%7C8dcbb865725d4151564408d7aa7893c9%7Cfa7b1b5a7b34438794aed2c178decee1%7C0%7C0%7C637165307165113089&sdata=a5evE514IT0GdVvL7w5IbOCcnyb%2FCwCqgxhOIlJsFvM%3D&reserved=0 > > > > > > > > > > > > > > > > On Thu, Jan 16, 2020 at 12:59 PM Josh Tynjala < > > > [email protected]> > > > > wrote: > > > > > > > > > Thank you, Harbs! Wrapping the variable name in a > > > > > goog.reflect.objectProperty() call works > perfectly. This > > is > > > exactly > > > why I > > > > > started this thread, to see if anyone could suggest > > possible > > > alternatives. > > > > > > > > > > Thankfully, we can keep the same simple data > structure as > > > before, > > > and my > > > > > initial proposal with functions can be forgotten. > In a > > release > > > build, I can > > > > > see that goog.reflect.objectProperty() calls are > > replaced by a > > > simple > > > > > string literal (containing the minified variable > name), > > so we > > > don't > > > have to > > > > > worry about extra performance impact. > > > > > > > > > > -- > > > > > Josh Tynjala > > > > > Bowler Hat LLC < > > > > > > > > > https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbowlerhat.dev&data=02%7C01%7Caharui%40adobe.com%7C8dcbb865725d4151564408d7aa7893c9%7Cfa7b1b5a7b34438794aed2c178decee1%7C0%7C0%7C637165307165113089&sdata=a5evE514IT0GdVvL7w5IbOCcnyb%2FCwCqgxhOIlJsFvM%3D&reserved=0 > > > > > > > > > > > > > > > > > > > On Wed, Jan 15, 2020 at 8:32 PM Harbs < > > [email protected]> > > > wrote: > > > > > > > > > >> Sounds good! > > > > >> > > > > >> > > > > >> > > > > > > > > > https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fgoogle%2Fclosure-compiler%2Fwiki%2FType-Based-Property-Renaming&data=02%7C01%7Caharui%40adobe.com%7C8dcbb865725d4151564408d7aa7893c9%7Cfa7b1b5a7b34438794aed2c178decee1%7C0%7C0%7C637165307165113089&sdata=O5PMzmZaDJnnWfkJL1EQMlGudNjREP88VIbImidkXtw%3D&reserved=0 > > > > >> < > > > > >> > > > > > > > > > https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fgoogle%2Fclosure-compiler%2Fwiki%2FType-Based-Property-Renaming&data=02%7C01%7Caharui%40adobe.com%7C8dcbb865725d4151564408d7aa7893c9%7Cfa7b1b5a7b34438794aed2c178decee1%7C0%7C0%7C637165307165113089&sdata=O5PMzmZaDJnnWfkJL1EQMlGudNjREP88VIbImidkXtw%3D&reserved=0 > > > > >> > > > > > >> > > > > >> The function seems to be > goog.reflect.objectProperty() > > > > >> > > > > >> I’m not sure exactly how it works though. > > > > >> > > > > >> > On Jan 16, 2020, at 1:37 AM, Greg Dove < > > [email protected] > > > > > > > wrote: > > > > >> > > > > > >> > actually just as another fyi, Harbs pointed out > some > > > intriguing > > > goog > > > > >> > methods recently - I don't have an immediate > > reference to it > > > sorry. One > > > > >> of > > > > >> > those seemed to allow for access to renamed > names by > > > wrapping the > > > > >> original > > > > >> > names in a 'magic' method that presumably GCC > > recognises > > > (but > > > presumably > > > > >> > returns the name unchanged in debug mode) > > > > >> > > > > > >> > > > > > >> > On Thu, Jan 16, 2020 at 12:33 PM Greg Dove < > > > [email protected]> > > > wrote: > > > > >> > > > > > >> >> reflection data has similar stuff to support > release > > mode > > > get/set for > > > > >> >> public vars. > > > > >> >> > > > > >> >> I did not look at MXML startup assignments like > > this, but > > > it > > > sounds > > > > >> good > > > > >> >> to me. I don't know if it makes sense, but > > considering > > > this is > > > just > > > > >> startup > > > > >> >> assignments, could one function combine all of > the > > startup > > > assignments > > > > >> (in > > > > >> >> the same sequence as before)? > > > > >> >> > > > > >> >> > > > > >> >> On Thu, Jan 16, 2020 at 12:23 PM Josh Tynjala < > > > > >> [email protected]> > > > > >> >> wrote: > > > > >> >> > > > > >> >>> According to the commit linked below, the > > > -warn-public-vars > > > compiler > > > > >> >>> option > > > > >> >>> was added because setting a public var in > MXML does > > not > > > currently work > > > > >> >>> properly in a release build. > > > > >> >>> > > > > >> >>> > > > > >> >>> > > > > >> > > > > > > > > > https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fapache%2Froyale-compiler%2Fcommit%2Feed5882ba935870a98ba4fe8cbf499e5d8344f60&data=02%7C01%7Caharui%40adobe.com%7C8dcbb865725d4151564408d7aa7893c9%7Cfa7b1b5a7b34438794aed2c178decee1%7C0%7C0%7C637165307165113089&sdata=M%2BEvAma5g8ufQL0Nf2Krb3hrr2ERTHhdsofqVUhm7f4%3D&reserved=0 > > > > >> >>> > > > > >> >>> In other words, this MXML code won't work if > it's a > > public > > > variable > > > > >> and > > > > >> >>> not > > > > >> >>> a setter: > > > > >> >>> > > > > >> >>> <Component publicVar="value"/> > > > > >> >>> > > > > >> >>> For reference, the compiler currently writes > the > > name of > > > the > > > public > > > > >> >>> variable as a string to the generated JS, > like this: > > > > >> >>> > > > > >> >>> var data = [ > > > > >> >>> Component, > > > > >> >>> 1, > > > > >> >>> 'publicVar', > > > > >> >>> true, > > > > >> >>> 'value' > > > > >> >>> ] > > > > >> >>> > > > > >> >>> At runtime, it interprets this array of > properties, > > and > > > basically runs > > > > >> >>> code > > > > >> >>> like this: > > > > >> >>> > > > > >> >>> comp['publicVar'] = 'value'; > > > > >> >>> > > > > >> >>> Since Closure compiler rewrites variable names > > during the > > > minification > > > > >> >>> process, this code keeps using the original > name, > > but > > > other > > > code in > > > > >> the > > > > >> >>> app > > > > >> >>> might start looking for a shorter variable > name > > like "uB". > > > This is the > > > > >> >>> failure that we're warning about. > > > > >> >>> > > > > >> >>> I propose updating the code generated by the > > compiler to > > > something > > > > >> like > > > > >> >>> this instead: > > > > >> >>> > > > > >> >>> var data = [ > > > > >> >>> Component, > > > > >> >>> 1, > > > > >> >>> function(){ this.publicVar=true } > > > > >> >>> ] > > > > >> >>> > > > > >> >>> At runtime, the class that interprets MXML > data will > > > detect the > > > > >> function > > > > >> >>> and call it like this: > > > > >> >>> > > > > >> >>> func.apply(comp); > > > > >> >>> > > > > >> >>> Because this new code will no longer use a > string, > > > Closure can > > > > >> rewrite the > > > > >> >>> property name with its minified version, just > like > > in > > > other > > > parts of > > > > >> the > > > > >> >>> app, and we'll no longer need to warn on > > declarations of > > > public > > > > >> variables. > > > > >> >>> > > > > >> >>> I have a working prototype for primitive > values, > > like > > > String, > > > > >> Boolean, and > > > > >> >>> Number. Objects and Arrays follow a different > path > > in the > > > MXML > > > data > > > > >> >>> interpreter, but I don't see why I wouldn't > be able > > to > > > handle > > > those > > > > >> with a > > > > >> >>> similar approach. > > > > >> >>> > > > > >> >>> Thoughts? > > > > >> >>> > > > > >> >>> -- > > > > >> >>> Josh Tynjala > > > > >> >>> Bowler Hat LLC < > > > > > > > > > https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbowlerhat.dev&data=02%7C01%7Caharui%40adobe.com%7C8dcbb865725d4151564408d7aa7893c9%7Cfa7b1b5a7b34438794aed2c178decee1%7C0%7C0%7C637165307165113089&sdata=a5evE514IT0GdVvL7w5IbOCcnyb%2FCwCqgxhOIlJsFvM%3D&reserved=0 > > > > > > > > >> >>> > > > > >> >> > > > > >> > > > > >> > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
