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" <joshtynj...@bowlerhat.dev> 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%7Cb7700e87eb404fce199308d7aa7368fb%7Cfa7b1b5a7b34438794aed2c178decee1%7C0%7C0%7C637165286896410173&sdata=lMDPEZBrk7kzWKr5MobPjR7R%2F1gFsWNJxoEJptTQxqA%3D&reserved=0>
On Sun, Jan 19, 2020 at 4:58 PM Alex Harui <aha...@adobe.com.invalid> 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" <joshtynj...@bowlerhat.dev> wrote:
>
> Comments inline.
>
> On Thursday, January 16, 2020, Alex Harui <aha...@adobe.com.invalid>
> 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" <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%7Cb7700e87eb404fce199308d7aa7368fb%7Cfa7b1b5a7b34438794aed2c178decee1%7C0%7C0%7C637165286896410173&sdata=lMDPEZBrk7kzWKr5MobPjR7R%2F1gFsWNJxoEJptTQxqA%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%7Cb7700e87eb404fce199308d7aa7368fb%7Cfa7b1b5a7b34438794aed2c178decee1%7C0%7C0%7C637165286896410173&sdata=lMDPEZBrk7kzWKr5MobPjR7R%2F1gFsWNJxoEJptTQxqA%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%7Cb7700e87eb404fce199308d7aa7368fb%7Cfa7b1b5a7b34438794aed2c178decee1%7C0%7C0%7C637165286896410173&sdata=xbvdLH%2FKesfH%2BXLc8AIOdsUsoeS%2BF6hMBiVV3fDXHIc%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%7Cb7700e87eb404fce199308d7aa7368fb%7Cfa7b1b5a7b34438794aed2c178decee1%7C0%7C0%7C637165286896410173&sdata=xbvdLH%2FKesfH%2BXLc8AIOdsUsoeS%2BF6hMBiVV3fDXHIc%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%7Cb7700e87eb404fce199308d7aa7368fb%7Cfa7b1b5a7b34438794aed2c178decee1%7C0%7C0%7C637165286896410173&sdata=Mr4EdF%2Bm%2B3T%2BAskoYeXXlle5PnSgON%2B5jgPC%2F0Zxr0w%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%7Cb7700e87eb404fce199308d7aa7368fb%7Cfa7b1b5a7b34438794aed2c178decee1%7C0%7C0%7C637165286896410173&sdata=lMDPEZBrk7kzWKr5MobPjR7R%2F1gFsWNJxoEJptTQxqA%3D&reserved=0
> >
> > >> >>>
> > >> >>
> > >>
> > >>
> >
> >
> >
>
>
>
