Just circling back to this: sure, I understand your point, Alex. I also am
usually all for addressing things in a central location.
I can see advantages either way, but had assumed that longer term it may be
advantageous to keep the AST/Typed AST more 'pure' and have 'Bindable' (and
possibly other forms of future metadata-driven) output variations be some
form of transformation at output. I am not only not a compiler expert, I'd
consider myself only slightly more than a novice (I did get my head into
the haxe compiler at one point a few years back, but ). So the way I think
about things is more in terms of analogies of things I do understand, which
may perhaps not be appropriate here.
I agree with what you pointed out for code implications and making future
maintenance/enhancements easier. I will give some thought to this in the
coming weeks, as maybe there is a way to refactor this to make things
easier all round, but still keep the AST 'pure'-ish. I can discuss with you
closer to the time, if so.
Re : "It is a convenience feature: the compiler could just report an error
saying you can't use [Bindable] on Objects."
I guess that is always another option. :)
[Bindable] implicit implementation was many times overused by some Flex
developers in the past, I was probably guilty of this myself in the early
days. But it is quite handy and quick sometimes, so long as you know the
implications of its use/abuse. And if it is taken away now I am pretty sure
sdk users will ask for it back ;)
For the patch mentioned below, will you apply that to develop? Or do you
want me to do that sometime tomorrow?
It sounded like you were ok with it, although had not tested it yet - fyi I
did run it by a few of the example apps and made sure the unit tests still
passed etc. I also made sure it correctly generated a swf with
flash.events.EventDispatcher instead of
org.apache.flex.events.EventDispatcher configured - not for a FlexJS
actual binding test which I think may not work in some cases because of
coercion issues in the framework classes, but just to check that it handled
the config changes correctly and the swf output was valid when the base
class for the event dispatcher reverted to a 'native' player class.
cheers,
Greg
On Tue, Sep 6, 2016 at 4:49 PM, Alex Harui <aha...@adobe.com> wrote:
> I haven't tested the patch, looks reasonable.
>
> That said, the reason I tried hacking the AST in ASCompilationUnit was
> because I thought the "best" way would be to effectively fix up the source
> code as if it had been written properly in the first place. IOW, this
> code effectively looks for source code that does:
>
> [Bindable]
> SomeClass extends Object
> {
> }
>
> And makes it look like:
>
> [Bindable]
> SomeClass extends EventDispatcher
> {
> }
>
> As you know, there are other patterns for static binding. Or another way
> to think of it: if you do take the time to re-base your [Bindable] source
> code on EventDispatcher, none of this code in the compiler executes. It
> is a convenience feature: the compiler could just report an error saying
> you can't use [Bindable] on Objects.
>
> I'm not a compiler expert, but back when Falcon only produced SWFs, I
> guess it seemed reasonable to leave the AST alone and generate modified
> output, but with FalconJX leveraging the AST, I'm concerned about work
> that means that future compiler developers will have to know that the AST
> doesn't go straight to JS or whatever the output is. As we've seen, the
> JSDoc annotation was driven off the AST. What else might we someday want
> to change in the AST "conversion" that will require us knowing about this
> code in ClassDirectiveProcessor?
>
> Anyway, again, I don't see any hurry to go back and look into
> re-implementing this [Bindable] handling in ASCompilationUnit. At least
> we've recorded in the archives that there might be other ways to solve
> this problem.
>
> Thanks,
> -Alex
>
> On 9/5/16, 6:44 PM, "Greg Dove" <greg.d...@gmail.com> wrote:
>
> >I think I found a solution for the dependency issue I mentioned.
> >The inclusion of "ValueChangeEvent" when not otherwise specified in the
> >project provided a useful clue here! (Otherwise I suspect I would not have
> >found this)
> >I need to do a bit more testing, but I expect to issue a related PR for a
> >change in ClassDirectiveProcessor before too long.
> >
> >On Mon, Sep 5, 2016 at 10:05 PM, Greg Dove <greg.d...@gmail.com> wrote:
> >
> >> Alex, I did some testing with a minimal project, I did not even have to
> >> try as-only, as SimpleApplication was sufficient. And yes, there is an
> >> issue if the dependency is not explicitly added somewhere. I will try to
> >> find a way to resolve this, otherwise the ASCompilationUnit code will
> >>need
> >> to go back.
> >> I had thought it might be useful to have the bindable implementation be
> >> more of any output variation than a manipulation of the AST. I had even
> >> wondered about longer term possibilities with such an approach - e.g.
> >> perhaps different binding implementation variations (e.g. as3 Signals
> >> instead of events or something like that). I realize that this is not
> >> something to think about seriously now. Plenty to do with things as they
> >> are... was just thinking of possibilities for the future.
> >>
> >> Anyhow, if I can't find a quick solution to add a specific dependency
> >>for
> >> output in the swf without the ASCompilationUnit AST manipulation
> >>approach,
> >> then that will need to go back (at least for now, anyway).
> >> Perhaps this was the reason you did this in the first place? Was it when
> >> the EventDispatcher base class for binding became non-native?
> >> If you think its best to try and fix the original problems with the
> >> original code in ASCompilationUnit , then perhaps that is the simplest
> >> route for now. If so, I'd be keen to leave the other stuff I added for a
> >> short time, but will happily remove the unnecessary parts once we can be
> >> sure they never get branched into as a fallback (i.e. we can be sure the
> >> ASCompilationUnit code catches all the 'extends' candidates). Let me
> >>know
> >> what you think.
> >>
> >> fyi I will be focusing on reflection work in whatever spare time I can
> >> find this week, following on from where you got to earlier this year
> >>with
> >> that. I will let you know where I get to later this week, I'm not sure
> >>if I
> >> will have it finished this week or not.
> >>
> >> cheers
> >> Greg
> >>
> >>
> >> On Sat, Sep 3, 2016 at 1:41 PM, Greg Dove <greg.d...@gmail.com> wrote:
> >>
> >>> Sound great. Yes, switching it back on at any point should remain a
> >>>quick
> >>> fix option because it would simply make the other code act as a
> >>>backstop.
> >>> I will take a quick look on Monday to see if I can find any problems in
> >>> small as-only projects where maybe the order of class definitions in
> >>>the
> >>> swf could be important with the current approach. Sounds a bit like a
> >>> theoretical edge case but you made me think about it.
> >>> Have a nice weekend.
> >>>
> >>> -Greg
> >>> [sent from my phone]
> >>>
> >>> On 3/09/2016 12:41 PM, "Alex Harui" <aha...@adobe.com> wrote:
> >>>
> >>>>
> >>>>
> >>>> On 9/2/16, 5:10 PM, "Greg Dove" <greg.d...@gmail.com> wrote:
> >>>>
> >>>> >Yes, that was one of the major changes. I moved it alongside the
> >>>>other
> >>>> >implementation inside ClassDirectiveProcessor and was able to get
> >>>>more
> >>>> >consistent results.
> >>>> >I initially commented it out so I could get everything working using
> >>>>the
> >>>> >implements IEventDispatcher approach - which is the more general
> >>>> approach
> >>>> >that would work for all cases- in javascript, then added it back for
> >>>> both
> >>>> >targets as a 2nd output variation in a later commit, and it went to
> >>>> >ClassDirectorProcessor for swf.
> >>>> >
> >>>> >I was seeing 2 issues from the ASCompilationUnit implementation:
> >>>> >1. It was not always applying where it should have been (so in cases
> >>>> where
> >>>> >it could have been 'extends EventDispatcher' it would sometimes fall
> >>>> >through to the implements IEventDispatcher approach in
> >>>> >ClassDIrectiveProcessor anyway). I suspect this was
> >>>>threading-related,
> >>>> but
> >>>> >I am not so familiar with threads.
> >>>> >
> >>>> >2. Less of an issue, it could also apply an EventDispatcher base
> >>>>class
> >>>> in
> >>>> >static-only bindable cases, which is unnecessary, this I expect could
> >>>> have
> >>>> >been more easily fixed. I tried checking with hasModifier in the
> >>>> original
> >>>> >code, but I think that might not be available yet, iirc that caused
> >>>>an
> >>>> >error.
> >>>> >
> >>>> >So I moved everything to the ClassDirectiveProcessor, which had the
> >>>> other
> >>>> >implementation of bindable support in any case. Sorry, I thought I
> >>>>had
> >>>> >been
> >>>> >clear about that.
> >>>>
> >>>> Well, you probably did explain it, but it probably didn't stick in my
> >>>> head
> >>>> until we hit this last issue and I went looking for the code where I
> >>>>had
> >>>> a
> >>>> vague memory of hacking a base class. I still don't know the compiler
> >>>> code so well that it is clear in my head what work should be done
> >>>>where.
> >>>> It looks like the original attempt to deal with [Bindable] for SWF was
> >>>> done in ClassDirectiveProcessor, but when I wanted to fix a bug in
> >>>> FalconJX I decided to fix it by hacking the AST since that drives
> >>>> everything in FalconJX. The right answer may have been for me to move
> >>>> the
> >>>> code from ClassDirectiveProcessor into ASCompilationUnit and use the
> >>>> existing tests for needsEventDispatcher, then maybe we wouldn't have
> >>>>had
> >>>> the two issues above.
> >>>>
> >>>> Anyway, I think we have recorded the possibility that AST hacking
> >>>>might
> >>>> be
> >>>> a solution if we hit other problems. I'm ok with leaving the code as
> >>>>is
> >>>> until we hit another problem but feel free to keep digging if you
> >>>>want.
> >>>>
> >>>> Thanks,
> >>>> -Alex
> >>>>
> >>>>
> >>
>
>
