My initial implementation will probably all or nothing, in terms of renaming, but we can revisit after to add more fine-grained control, if necessary.
I'll try to take a look at that constant issue when I get a chance. -- Josh Tynjala Bowler Hat LLC <https://bowlerhat.dev> On Wed, Feb 5, 2020 at 12:23 PM Harbs <[email protected]> wrote: > I hope there will be an option to prevent specific variables. > > The obvious ones would be ones used in MXML. > > Other candidates would be classes which correspond to JSON so the classes > can be constructed with bracket notation. I’m not sure of the best way to > declare a class for that. Maybe abusing “dynamic” classes? > > FWIW, I also noticed a bug recently that I didn’t log: > > In todomvc there’s the following in the router branch: > > <j:states> > <js:State name="All"/> > <js:State name="Active"/> > <js:State name="Completed"/> > </j:states> > > I tried using constants like so, and it failed. (At runtime the values > were “ALL”,”ACTIVE_FILTER” and “COMPLETED_FILTER”.) > <j:states> > <js:State name="{TodoModel.ALL}"/> > <js:State name="{TodoModel.ACTIVE_FILTER}"/> > <js:State name="{TodoModel.COMPLETED_FILTER}"/> > </j:states> > > > > On Feb 5, 2020, at 9:41 PM, 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://bowlerhat.dev> > > > > > > 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%7C1e9c2c4ae9d049b2896708d79b7843c7%7Cfa7b1b5a7b34438794aed2c178decee1%7C0%7C0%7C637148813154391097&sdata=8T%2B4UIVZrakKMBkAs2qOcwalYaCVJuMxHMKPYTnbvxM%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%7C1e9c2c4ae9d049b2896708d79b7843c7%7Cfa7b1b5a7b34438794aed2c178decee1%7C0%7C0%7C637148813154391097&sdata=8T%2B4UIVZrakKMBkAs2qOcwalYaCVJuMxHMKPYTnbvxM%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%7C1e9c2c4ae9d049b2896708d79b7843c7%7Cfa7b1b5a7b34438794aed2c178decee1%7C0%7C0%7C637148813154391097&sdata=IzzY7%2BhSQ9iQr9GIKd16XD%2FfGwxB50aN6J0Mzb%2BEcWA%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%7C1e9c2c4ae9d049b2896708d79b7843c7%7Cfa7b1b5a7b34438794aed2c178decee1%7C0%7C0%7C637148813154391097&sdata=IzzY7%2BhSQ9iQr9GIKd16XD%2FfGwxB50aN6J0Mzb%2BEcWA%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%7C1e9c2c4ae9d049b2896708d79b7843c7%7Cfa7b1b5a7b34438794aed2c178decee1%7C0%7C0%7C637148813154391097&sdata=aA3JAnaQuU1GrF%2B4vmTo1Lhn38gDM4UtG2%2FdbmwBnjQ%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%7C1e9c2c4ae9d049b2896708d79b7843c7%7Cfa7b1b5a7b34438794aed2c178decee1%7C0%7C0%7C637148813154391097&sdata=8T%2B4UIVZrakKMBkAs2qOcwalYaCVJuMxHMKPYTnbvxM%3D&reserved=0 > >>> > >>>>>>>> > >>>>>>> > >>>>> > >>>>> > >>> > >>> > >>> > >> > >> > >> > >
