Update _EventStreamSubscription.cancel() to have the same timing with and without sound null safety #44157
Labels
area-web
Use area-web for Dart web related issues, including the DDC and dart2js compilers and JS interop.
NNBD
Issues related to NNBD Release
P2
A bug or feature request we're likely to work on
web-libraries
Issues impacting dart:html, etc., libraries
Comments
nshahan
added
web-libraries
dart-bot
pushed a commit
that referenced
this issue
Nov 16, 2020
Casts in _EventStreamSubscription.cancel() are causing unwanted failures in ddc when we turn weak null safety warnings into errors via a flag. By removing the casts more apps can run when the weak mode errors are enabled. The null value is used inside the SDK to signal synchronous behavior. This is a temporary fix until we can resolve the larger issue with the API in dart:html. See #44157. Change-Id: Ic9651779a394a03f092a5b4b7944c1306afcb826 Issue: #41653 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/171660 Reviewed-by: Mark Zhou <markzipan@google.com> Commit-Queue: Nicholas Shahan <nshahan@google.com>
dart-bot
pushed a commit
that referenced
this issue
Jan 7, 2021
Before Null Safety, `_EventStreamSubscription.cancel()` used a trick to run with synchronous timing even though it was typed to return a `Future`. During the migration of the SDK to support Null Safety it kept the synchronous timing in weak mode, but was changed to asynchronous in sound mode so that the behavior matched the method signature. In hindsight, changing the timing when opting into Null Safety is problematic: * A shared package has no control over what mode it runs in. Libraries may be opted in and run their tests with sound null safety but the apps they are used in could still be running in weak mode. This results in library unit tests that behave differently than the production app that deploys the code. * This codepath can be triggered by EventTarget.dispatchEvent() from dart:html which should have synchronous timings for the event listeners before returning to the calling code. The asynchronous timing when running with sound null safety is inconsistent with the browser API. This change reverses that migration decision and keeps the synchronous timing in both modes. To support this in sound mode it returns a special future value that is internal to the SDK and known to be used for synchronous timing. This change also removes the workaround introduced in DDC to avoid warning/failing when `_EventStreamSubscription.cancel()` returned null and the extra warnings/errors features were enabled in weak null safety mode. Change-Id: I6b08a2ada5b10120bea787ad59d1d58e6e181de5 Fixes: #44157 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/175323 Commit-Queue: Nicholas Shahan <nshahan@google.com> Reviewed-by: Srujan Gaddam <srujzs@google.com> Reviewed-by: Sigmund Cherem <sigmund@google.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
In #41653 we decided to hide a breaking timing change behind opting into sound null safety. This has proven problematic for multiple reasons.
EventTarget.dispatchEvent()which should have synchronous timings for the event listeners before returning to the calling code. The asynchronous timing when running with sound null safety might actually not be correct for this use case.null(to signal the synchronous timing) in weak mode when the method is statically typed to return a non-nullableFuturegives unwanted warnings/errors from the features DDC offers to assist in large scale migrations.The text was updated successfully, but these errors were encountered: