You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@royale.apache.org by Alex Harui <ah...@adobe.com.INVALID> on 2018/02/17 05:40:10 UTC
-remove-circulars
Hi Folks,
I've spent the last day or so trying to get ASDoc to run when minified. I
created a JSON to ValueObjects utility which helped a lot. It still isn't
completely running, but it appears that we need to stop pruning classes
that are only used in type annotations. This will make most apps a little
fatter, but also require all apps to use remove-circulars.
What do folks think about that? The compiler used to prune classes from a
file's goog.requires list that were never instantiated or type-checked in
that file. So, for ASDoc, the ASDocClass ValueObject that represents the
JSON data for the ASDoc is never instantiated by the class that consumes
it. The instances are created by JSON. The ASDocClass is only mentioned
in JSDoc to declare some variable as being of type ASDoc. That enabled us
to remove that class from the requires list since that ASDocClass is never
referenced by operational code. Removing that goog.require helped
eliminate circular goog.require references that the closure compiler
disallows. That kicked HTMLElementWrapper out of the app for example.
But it appears that by removing that goog.require, the compiler didn't
know that the ASDocClass properties were exported and it renamed some of
them resulting in problems in js-release. So by leaving more
goog.requires in the code we get better support against renaming, but we
also bring back more circularities and now even HelloWorld would need
-remove-circulars.
An alternative that would allow a few more apps to not need
remove-circulars is to modify a few of our examples and classes to
eliminate circularities. HelloWorld only has 3. But 2 of them are
IParent/IChild and IStrand/IBead. It seems "right" to have IParent
reference IChild and vice versa. Same for IStrand and IBead. I'm not
quite sure what the answer is. Maybe IParent can reference IChild, but
IChild does not reference its IParent. I guess you could make an
IParentedChild interface with the parent property and concrete children
implement both IChild and IParentedChild. That seems wrong and overly
complicated.
Yet another option, which is recommended by some folks in the Closure
Compiler community but doesn't fit well with the ActionScript language is
to define both IChild and IParent in the same file. I do not want to
spend the time modifying the compiler for that. It might be practical to
merge the files as part of the Ant and Maven builds. But again, that will
take time as well.
That said, requiring remove-circulars always doesn't seem quite right
either.
Thoughts?
-Alex
Re: -remove-circulars
Posted by Piotr Zarzycki <pi...@gmail.com>.
Hi Alex,
I looks serious. Maybe we szould provide some custom option for compiler.
If someone will have the problem with js-release he will switch up. Behind
the stage remove circular will be also ON.
I do not understand actually how do you suggest to do not remove some
classes? Because if Inunderstand correctly that's what happenning here -
they are removed because they are not used directly.
I have to admit that it may help to one of my example which simply doesn't
work in release version.
Thanks,
Piotr
On Sat, Feb 17, 2018, 06:40 Alex Harui <ah...@adobe.com.invalid> wrote:
> Hi Folks,
>
> I've spent the last day or so trying to get ASDoc to run when minified. I
> created a JSON to ValueObjects utility which helped a lot. It still isn't
> completely running, but it appears that we need to stop pruning classes
> that are only used in type annotations. This will make most apps a little
> fatter, but also require all apps to use remove-circulars.
>
> What do folks think about that? The compiler used to prune classes from a
> file's goog.requires list that were never instantiated or type-checked in
> that file. So, for ASDoc, the ASDocClass ValueObject that represents the
> JSON data for the ASDoc is never instantiated by the class that consumes
> it. The instances are created by JSON. The ASDocClass is only mentioned
> in JSDoc to declare some variable as being of type ASDoc. That enabled us
> to remove that class from the requires list since that ASDocClass is never
> referenced by operational code. Removing that goog.require helped
> eliminate circular goog.require references that the closure compiler
> disallows. That kicked HTMLElementWrapper out of the app for example.
> But it appears that by removing that goog.require, the compiler didn't
> know that the ASDocClass properties were exported and it renamed some of
> them resulting in problems in js-release. So by leaving more
> goog.requires in the code we get better support against renaming, but we
> also bring back more circularities and now even HelloWorld would need
> -remove-circulars.
>
> An alternative that would allow a few more apps to not need
> remove-circulars is to modify a few of our examples and classes to
> eliminate circularities. HelloWorld only has 3. But 2 of them are
> IParent/IChild and IStrand/IBead. It seems "right" to have IParent
> reference IChild and vice versa. Same for IStrand and IBead. I'm not
> quite sure what the answer is. Maybe IParent can reference IChild, but
> IChild does not reference its IParent. I guess you could make an
> IParentedChild interface with the parent property and concrete children
> implement both IChild and IParentedChild. That seems wrong and overly
> complicated.
>
> Yet another option, which is recommended by some folks in the Closure
> Compiler community but doesn't fit well with the ActionScript language is
> to define both IChild and IParent in the same file. I do not want to
> spend the time modifying the compiler for that. It might be practical to
> merge the files as part of the Ant and Maven builds. But again, that will
> take time as well.
>
> That said, requiring remove-circulars always doesn't seem quite right
> either.
>
> Thoughts?
> -Alex
>
>
>
>
Re: -remove-circulars
Posted by Alex Harui <ah...@adobe.com.INVALID>.
I turned on -remove-circulars by default for now so I could push other
changes. ASDoc now appears to be working in js-release. I am going to
push the js-release version to the website.
-Alex
On 2/17/18, 8:21 PM, "Alex Harui" <ah...@adobe.com.INVALID> wrote:
>
>
>On 2/17/18, 9:00 AM, "Gabe Harbs" <ha...@gmail.com> wrote:
>
>>FWIW, none of the apps I built to date compiled correctly without
>>-remove-circulars.
>>
>>I’m not sure I completely understand the issue. Will your proposed
>>solution make minified apps fatter, or just the debug apps?
>
>Good question. I think you might be right that the optimizer will kick
>out those classes anyway. Those data classes will likely just be used to
>make the closure compiler happy and prevent renaming.
>>
>>Can you point me to where in the ASDoc code the problem is so In can
>>understand the problem better?
>
>ASDocModel manages the JSON results. What is in the repo doesn't contain
>the ValueObjects I'm now working with, but I have a JSONReviver class that
>converts JSON to VOs on the fly. The JSON works from registered
>[RemoteClasses] similar to how AMF works. That means, if I have a class
>called ASDocClass that represents the ASDoc for a Class, there is no code
>my local version of ASDocModel that calls "new ASDocClass". Instead, it
>just does:
>
> var data:ASDocClass = app.reviver.parse(app.service.data) as
>ASDocClass;
>
>
>Then, when optimizing "data.members", if the compiler has already removed
>the goog.require('ASDocClass') then the Closure Compiler might rename
>members to "xy".
>
>The compiler currently tries to detect how you used a class in a file to
>decide whether it needs to be goog.require'd or not. The current
>ASDocModel code also has an "@flexjsignorecoercion ASDocClass" so the JS
>output of the line above is:
>
> var /** @type {ASDocClass} */ data =
>this.app.reviver.parse(this.app.service.data);
>
>Since this code can run without needing an ASDocClass definition, the
>current compiler removes the goog.require. This helps reduce the number
>of circularities in Flex since simple parent/child or bead/strand always
>always has at least one class in the pair reference the other class as
>only a type. For example:
>
>interface IParent {
> var child:IChild;
>}
>
>interface IChild {
> var parent:IParent;
>}
>
>The JS output is, more or less:
>
>goog.provide('IParent');
>goog.require('IChild');
>IParent = function();
>/** @type {IChild} */
>IParent.prototype.IChild;
>
>goog.provide('IChild');
>goog.require('IParent');
>IChild = function();
>/** @type {IParent} */
>IChild.prototype.IParent;
>
>
>Again, you can see that IParent doesn't have to exist at runtime to make
>IChild happy and vice versa. But the Closure Compiler will see the
>goog.requires as a circularity, so that's why the current Royale Compiler
>found a way to remove them. I don't know a way to detect that a
>goog.require is only used for type-checking and not later used to prevent
>renaming. So the change I made was to turn off the pruning of
>goog.requires. I think this will help prevent renaming in more places and
>make the js-release version work more often. But now, every app will use
>IParent and IBead and result in circularities from Closure Compiler.
>
>The reason -remove-circulars is not on by default is that some folks
>wanted to be able to write apps that don't have circularities so they want
>Closure Compiler to complain so they can fix their code by breaking the
>circularities. Also note that -remove-circulars also removes
>goog.requires and thus still presents a risk that closure compiler will
>rename something it shouldn't. -remove-circulars chooses to remove
>goog.requires that have already been seen by a dependency walk starting
>with the main class. The circularity removal code uses a different
>criteria for deciding whether to remove a goog.require. It will remove a
>goog.require unless it sees it is used in a static variable initializer.
>
>So, there is a chance that as we continue to debug js-release versions we
>will see that we must break circularities the theoretical way instead of
>by using -remove-circulars. We might just be getting lucky so far.
>
>I am devoting time to this now not only to get ASDoc running but to get a
>better understanding of why js-release versions don't work so we know what
>to look for when we debug really big apps some day.
>
>What I'm working on now is changing over some "public var foo" to
>getter/setters in order to address renaming issues for properties
>referenced by MXML code. I'm probably going to add a warning to the
>compiler on every use of "public var" to help us sweep the code and
>convert to getter/setter to ward off renaming issues later.
>
>HTH,
>-Alex
>
>>
>>Harbs
>>
>>> On Feb 17, 2018, at 7:40 AM, Alex Harui <ah...@adobe.com.INVALID>
>>>wrote:
>>>
>>> Hi Folks,
>>>
>>> I've spent the last day or so trying to get ASDoc to run when minified.
>>> I
>>> created a JSON to ValueObjects utility which helped a lot. It still
>>>isn't
>>> completely running, but it appears that we need to stop pruning classes
>>> that are only used in type annotations. This will make most apps a
>>>little
>>> fatter, but also require all apps to use remove-circulars.
>>>
>>> What do folks think about that? The compiler used to prune classes
>>>from a
>>> file's goog.requires list that were never instantiated or type-checked
>>>in
>>> that file. So, for ASDoc, the ASDocClass ValueObject that represents
>>>the
>>> JSON data for the ASDoc is never instantiated by the class that
>>>consumes
>>> it. The instances are created by JSON. The ASDocClass is only
>>>mentioned
>>> in JSDoc to declare some variable as being of type ASDoc. That enabled
>>>us
>>> to remove that class from the requires list since that ASDocClass is
>>>never
>>> referenced by operational code. Removing that goog.require helped
>>> eliminate circular goog.require references that the closure compiler
>>> disallows. That kicked HTMLElementWrapper out of the app for example.
>>> But it appears that by removing that goog.require, the compiler didn't
>>> know that the ASDocClass properties were exported and it renamed some
>>>of
>>> them resulting in problems in js-release. So by leaving more
>>> goog.requires in the code we get better support against renaming, but
>>>we
>>> also bring back more circularities and now even HelloWorld would need
>>> -remove-circulars.
>>>
>>> An alternative that would allow a few more apps to not need
>>> remove-circulars is to modify a few of our examples and classes to
>>> eliminate circularities. HelloWorld only has 3. But 2 of them are
>>> IParent/IChild and IStrand/IBead. It seems "right" to have IParent
>>> reference IChild and vice versa. Same for IStrand and IBead. I'm not
>>> quite sure what the answer is. Maybe IParent can reference IChild, but
>>> IChild does not reference its IParent. I guess you could make an
>>> IParentedChild interface with the parent property and concrete children
>>> implement both IChild and IParentedChild. That seems wrong and overly
>>> complicated.
>>>
>>> Yet another option, which is recommended by some folks in the Closure
>>> Compiler community but doesn't fit well with the ActionScript language
>>>is
>>> to define both IChild and IParent in the same file. I do not want to
>>> spend the time modifying the compiler for that. It might be practical
>>>to
>>> merge the files as part of the Ant and Maven builds. But again, that
>>>will
>>> take time as well.
>>>
>>> That said, requiring remove-circulars always doesn't seem quite right
>>> either.
>>>
>>> Thoughts?
>>> -Alex
>>>
>>>
>>>
>>
>
Re: -remove-circulars
Posted by Alex Harui <ah...@adobe.com.INVALID>.
On 2/17/18, 9:00 AM, "Gabe Harbs" <ha...@gmail.com> wrote:
>FWIW, none of the apps I built to date compiled correctly without
>-remove-circulars.
>
>I’m not sure I completely understand the issue. Will your proposed
>solution make minified apps fatter, or just the debug apps?
Good question. I think you might be right that the optimizer will kick
out those classes anyway. Those data classes will likely just be used to
make the closure compiler happy and prevent renaming.
>
>Can you point me to where in the ASDoc code the problem is so In can
>understand the problem better?
ASDocModel manages the JSON results. What is in the repo doesn't contain
the ValueObjects I'm now working with, but I have a JSONReviver class that
converts JSON to VOs on the fly. The JSON works from registered
[RemoteClasses] similar to how AMF works. That means, if I have a class
called ASDocClass that represents the ASDoc for a Class, there is no code
my local version of ASDocModel that calls "new ASDocClass". Instead, it
just does:
var data:ASDocClass = app.reviver.parse(app.service.data) as ASDocClass;
Then, when optimizing "data.members", if the compiler has already removed
the goog.require('ASDocClass') then the Closure Compiler might rename
members to "xy".
The compiler currently tries to detect how you used a class in a file to
decide whether it needs to be goog.require'd or not. The current
ASDocModel code also has an "@flexjsignorecoercion ASDocClass" so the JS
output of the line above is:
var /** @type {ASDocClass} */ data =
this.app.reviver.parse(this.app.service.data);
Since this code can run without needing an ASDocClass definition, the
current compiler removes the goog.require. This helps reduce the number
of circularities in Flex since simple parent/child or bead/strand always
always has at least one class in the pair reference the other class as
only a type. For example:
interface IParent {
var child:IChild;
}
interface IChild {
var parent:IParent;
}
The JS output is, more or less:
goog.provide('IParent');
goog.require('IChild');
IParent = function();
/** @type {IChild} */
IParent.prototype.IChild;
goog.provide('IChild');
goog.require('IParent');
IChild = function();
/** @type {IParent} */
IChild.prototype.IParent;
Again, you can see that IParent doesn't have to exist at runtime to make
IChild happy and vice versa. But the Closure Compiler will see the
goog.requires as a circularity, so that's why the current Royale Compiler
found a way to remove them. I don't know a way to detect that a
goog.require is only used for type-checking and not later used to prevent
renaming. So the change I made was to turn off the pruning of
goog.requires. I think this will help prevent renaming in more places and
make the js-release version work more often. But now, every app will use
IParent and IBead and result in circularities from Closure Compiler.
The reason -remove-circulars is not on by default is that some folks
wanted to be able to write apps that don't have circularities so they want
Closure Compiler to complain so they can fix their code by breaking the
circularities. Also note that -remove-circulars also removes
goog.requires and thus still presents a risk that closure compiler will
rename something it shouldn't. -remove-circulars chooses to remove
goog.requires that have already been seen by a dependency walk starting
with the main class. The circularity removal code uses a different
criteria for deciding whether to remove a goog.require. It will remove a
goog.require unless it sees it is used in a static variable initializer.
So, there is a chance that as we continue to debug js-release versions we
will see that we must break circularities the theoretical way instead of
by using -remove-circulars. We might just be getting lucky so far.
I am devoting time to this now not only to get ASDoc running but to get a
better understanding of why js-release versions don't work so we know what
to look for when we debug really big apps some day.
What I'm working on now is changing over some "public var foo" to
getter/setters in order to address renaming issues for properties
referenced by MXML code. I'm probably going to add a warning to the
compiler on every use of "public var" to help us sweep the code and
convert to getter/setter to ward off renaming issues later.
HTH,
-Alex
>
>Harbs
>
>> On Feb 17, 2018, at 7:40 AM, Alex Harui <ah...@adobe.com.INVALID>
>>wrote:
>>
>> Hi Folks,
>>
>> I've spent the last day or so trying to get ASDoc to run when minified.
>> I
>> created a JSON to ValueObjects utility which helped a lot. It still
>>isn't
>> completely running, but it appears that we need to stop pruning classes
>> that are only used in type annotations. This will make most apps a
>>little
>> fatter, but also require all apps to use remove-circulars.
>>
>> What do folks think about that? The compiler used to prune classes
>>from a
>> file's goog.requires list that were never instantiated or type-checked
>>in
>> that file. So, for ASDoc, the ASDocClass ValueObject that represents
>>the
>> JSON data for the ASDoc is never instantiated by the class that consumes
>> it. The instances are created by JSON. The ASDocClass is only
>>mentioned
>> in JSDoc to declare some variable as being of type ASDoc. That enabled
>>us
>> to remove that class from the requires list since that ASDocClass is
>>never
>> referenced by operational code. Removing that goog.require helped
>> eliminate circular goog.require references that the closure compiler
>> disallows. That kicked HTMLElementWrapper out of the app for example.
>> But it appears that by removing that goog.require, the compiler didn't
>> know that the ASDocClass properties were exported and it renamed some of
>> them resulting in problems in js-release. So by leaving more
>> goog.requires in the code we get better support against renaming, but we
>> also bring back more circularities and now even HelloWorld would need
>> -remove-circulars.
>>
>> An alternative that would allow a few more apps to not need
>> remove-circulars is to modify a few of our examples and classes to
>> eliminate circularities. HelloWorld only has 3. But 2 of them are
>> IParent/IChild and IStrand/IBead. It seems "right" to have IParent
>> reference IChild and vice versa. Same for IStrand and IBead. I'm not
>> quite sure what the answer is. Maybe IParent can reference IChild, but
>> IChild does not reference its IParent. I guess you could make an
>> IParentedChild interface with the parent property and concrete children
>> implement both IChild and IParentedChild. That seems wrong and overly
>> complicated.
>>
>> Yet another option, which is recommended by some folks in the Closure
>> Compiler community but doesn't fit well with the ActionScript language
>>is
>> to define both IChild and IParent in the same file. I do not want to
>> spend the time modifying the compiler for that. It might be practical
>>to
>> merge the files as part of the Ant and Maven builds. But again, that
>>will
>> take time as well.
>>
>> That said, requiring remove-circulars always doesn't seem quite right
>> either.
>>
>> Thoughts?
>> -Alex
>>
>>
>>
>
Re: -remove-circulars
Posted by Gabe Harbs <ha...@gmail.com>.
FWIW, none of the apps I built to date compiled correctly without -remove-circulars.
I’m not sure I completely understand the issue. Will your proposed solution make minified apps fatter, or just the debug apps?
Can you point me to where in the ASDoc code the problem is so In can understand the problem better?
Harbs
> On Feb 17, 2018, at 7:40 AM, Alex Harui <ah...@adobe.com.INVALID> wrote:
>
> Hi Folks,
>
> I've spent the last day or so trying to get ASDoc to run when minified. I
> created a JSON to ValueObjects utility which helped a lot. It still isn't
> completely running, but it appears that we need to stop pruning classes
> that are only used in type annotations. This will make most apps a little
> fatter, but also require all apps to use remove-circulars.
>
> What do folks think about that? The compiler used to prune classes from a
> file's goog.requires list that were never instantiated or type-checked in
> that file. So, for ASDoc, the ASDocClass ValueObject that represents the
> JSON data for the ASDoc is never instantiated by the class that consumes
> it. The instances are created by JSON. The ASDocClass is only mentioned
> in JSDoc to declare some variable as being of type ASDoc. That enabled us
> to remove that class from the requires list since that ASDocClass is never
> referenced by operational code. Removing that goog.require helped
> eliminate circular goog.require references that the closure compiler
> disallows. That kicked HTMLElementWrapper out of the app for example.
> But it appears that by removing that goog.require, the compiler didn't
> know that the ASDocClass properties were exported and it renamed some of
> them resulting in problems in js-release. So by leaving more
> goog.requires in the code we get better support against renaming, but we
> also bring back more circularities and now even HelloWorld would need
> -remove-circulars.
>
> An alternative that would allow a few more apps to not need
> remove-circulars is to modify a few of our examples and classes to
> eliminate circularities. HelloWorld only has 3. But 2 of them are
> IParent/IChild and IStrand/IBead. It seems "right" to have IParent
> reference IChild and vice versa. Same for IStrand and IBead. I'm not
> quite sure what the answer is. Maybe IParent can reference IChild, but
> IChild does not reference its IParent. I guess you could make an
> IParentedChild interface with the parent property and concrete children
> implement both IChild and IParentedChild. That seems wrong and overly
> complicated.
>
> Yet another option, which is recommended by some folks in the Closure
> Compiler community but doesn't fit well with the ActionScript language is
> to define both IChild and IParent in the same file. I do not want to
> spend the time modifying the compiler for that. It might be practical to
> merge the files as part of the Ant and Maven builds. But again, that will
> take time as well.
>
> That said, requiring remove-circulars always doesn't seem quite right
> either.
>
> Thoughts?
> -Alex
>
>
>