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. cheers Greg On Sat, Sep 3, 2016 at 11:55 AM, Alex Harui <aha...@adobe.com> wrote: > > > On 9/2/16, 4:30 PM, "Greg Dove" <greg.d...@gmail.com> wrote: > > >"In looking at the change in Falcon, it appears that the patch was > >required > >because the resolveBaseClass does not resolve to EventDispatcher. Did you > >look into trying to get resolveBaseClass to resolve to EventDispatcher. > >I'm wondering if there will be other issues related to that." > > > >The change is more at the output stage rather than hacking the original > >AST > >to do that now. You know this far better than I do, but I don't *think* > >this should cause an issue in js because of the way the framework classes > >are always avalalable, but just thinking about it, perhaps for a simple AS > >project with a couple of Bindable classes, if the implicit implementation > >is created this may cause an issue in swf, because the baseclass > >EventDispatcher is now non-native - if the dependency is not addressed > >explicitly elsewhere in the swf - is that what you mean? I haven't tried > >anything like this, but I can check this first thing on Monday my time. > > In looking more closely at these changes, one of the changes removed code > from ASCompilationUnit that set the base class to EventDispatcher. I > hadn't realized that code wasn't replaced elsewhere, but it seems like > hacking the AST to set the base class would have also fixed the last two > issues (the missing goog.require and the missing @extends). So now I'm > more curious about why you chose to move the logic to > ClassDirectiveProcessor, and that further makes me wonder if we just > haven't noticed some other ramification of not hacking the AST. > > We can continue this on Monday, no hurry. > > Thanks, > -Alex > >
