I'm not sure that I understand how this solution takes control away from anyone. It fixes a bug when a public variable is renamed, but at the same time, it preserves the existing behavior for things that are not renamed.
- Josh On Thursday, January 16, 2020, Alex Harui <aha...@adobe.com.invalid> wrote: > I'm not surprised that this change fixed a problem caused by removing @export. This is not a question about the technical accuracy of your work, which is always very good. This just doesn't feel like the right solution. Maybe we should start by agreeing on facts and then goals and then discuss solutions. This solution seems to take control of the output away from folks who will want different output and is solving a specific scenario and not the general problem in a less optimal way. > > 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. > 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 > 3) The Closure Compiler is open source and designed to be extended > 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. > 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. > 6) All getter/setters are not renamed, but are not exported. > 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. > > 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 > 9) Renaming can complicate and slow serialization/deserialization > > IMO, we want to be heading in the direction of A) allowing control over what gets renamed, B) capturing information from the compiler, C) controlling the set of renames and exports directly, not through the output. > > My 2 cents, > -Alex > > > On 1/16/20, 2:48 PM, "Josh Tynjala" <joshtynj...@bowlerhat.dev> 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%7Cc060c7977c184a07aa2708d79ad6357a%7Cfa7b1b5a7b34438794aed2c178decee1%7C0%7C0%7C637148117120345421&sdata=ysm%2FJ2FfEK9jKlj4gND5LIFLihlaP6pHZYs0eueIihs%3D&reserved=0 > > > > On Thu, Jan 16, 2020 at 12:59 PM Josh Tynjala < joshtynj...@bowlerhat.dev> > 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%7Cc060c7977c184a07aa2708d79ad6357a%7Cfa7b1b5a7b34438794aed2c178decee1%7C0%7C0%7C637148117120355415&sdata=6fT85c%2FQJ8aXEYf%2FQgZ%2BcoEj1%2F0%2BfmskfdvHXuLeXOg%3D&reserved=0 > > > > > > > On Wed, Jan 15, 2020 at 8:32 PM Harbs <harbs.li...@gmail.com> 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%7Cc060c7977c184a07aa2708d79ad6357a%7Cfa7b1b5a7b34438794aed2c178decee1%7C0%7C0%7C637148117120355415&sdata=ncKq3dWJbDBJB6EylRMugca0Ck512sngA6O6KQ9IupQ%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%7Cc060c7977c184a07aa2708d79ad6357a%7Cfa7b1b5a7b34438794aed2c178decee1%7C0%7C0%7C637148117120355415&sdata=ncKq3dWJbDBJB6EylRMugca0Ck512sngA6O6KQ9IupQ%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 <greg.d...@gmail.com> 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 <greg.d...@gmail.com> 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 < > >> joshtynj...@bowlerhat.dev> > >> >> 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%7Cc060c7977c184a07aa2708d79ad6357a%7Cfa7b1b5a7b34438794aed2c178decee1%7C0%7C0%7C637148117120355415&sdata=a3kRb8bAWJfR%2BiTmsdk11%2FfvdXgSyYrJXDFfBY7nSok%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%7Cc060c7977c184a07aa2708d79ad6357a%7Cfa7b1b5a7b34438794aed2c178decee1%7C0%7C0%7C637148117120355415&sdata=6fT85c%2FQJ8aXEYf%2FQgZ%2BcoEj1%2F0%2BfmskfdvHXuLeXOg%3D&reserved=0 > > >> >>> > >> >> > >> > >> > > > -- -- Josh Tynjala Bowler Hat LLC <https://bowlerhat.dev>
