Lifecycle events that are "async" cause subtle bugs

[matanlurey]

A lot of subtle bug across change detection have been narrowed down to the following:

DartPad.

class Comp  {
  ngOnInit() async {
    print('>>> ngOnInit()');
  }
}

class _View {
  detectChanges(Comp comp) {
    print('>>> 1');
    comp.ngOnInit();
    print('>>> 2');
  }
}

void main() {
  new _View().detectChanges(new Comp());
}

Expected Behavior

>>> 1
>>> ngOnInit()
>>> 2

Actual Behavior

>>> 1
>>> 2
>>> ngOnInit()

Discussion

Unfortunately, there is no framework specific way to fix this, as we can't force to run this synchronously, but the rest of the framework expects it to be (and users do too)! A couple of ideas:

1. Wait for the Dart language to change the semantics of an "async" function:
   Meta-issue for discussion of the proposal to make async functions start synchronously dart-lang/sdk#30345 - nothing has happened here yet we can use.
2. Change the signature of all the lifecycle events to void to disallow async (BREAKING)
3. Warn in the compilation process if we notice an async function.
4. Internally await asynchronous lifecycle events.
5. Add new lifecycle events that support async setup/teardown.

Unfortunately, (1) is outside of our immediate control, and (2) and (3) would prevent folks from using the async/await syntax in lifecycle methods, which is desirable to some. (4) would fix the usability of the issue, at the expense of making the currently synchronous change detection much more complicated and async, which is also not desirable.

I'm not 100% sure how (5) would work without usability issues.

[matanlurey]

One idea from internal discussion: change the return type to avoid with this as a hack:

void ngOnInit() => (() async {
  // Old ngOnInit body.
})();

[leafpetersen]

Currently that (the void returning async function) will be a strong mode error, but the take away from the discussion in dart-lang/sdk#31324 (comment) was that it shouldn't be.

[matanlurey]

@leafpetersen: Wait, this will be legal?

void ngOnInit() async {
  // ...
}

[leafpetersen]

@matanlurey Yes, that was my takeaway from the discussion in #31324. As I said there, I don't feel like I have strong arguments for or against it, and @lrhn seems to feel that it's useful. Happy to hear more feedback about it.

[lrhn]

We have changed the behavior for Dart 2, so async functions start running immediately, up to the first await. We did that exactly because of issues like this (someone changing a sync function to async would be surprised by the initial check not being done immediately, I've caught a few race conditions like that in reviews).
I believe @floitschG is still working on implementing that and updating existing code.

[matanlurey]

This is fixed enough with sync-async.