Skip to content

Conversation

@stuartmorgan-g
Copy link
Collaborator

@stuartmorgan-g stuartmorgan-g commented May 23, 2023

This option had been disabled to match flutter/flutter, but the reason it was disabled there was "too many false positives", mostly around animation. That doesn't apply to most packages here, and we've had a number of production bugs—especially in plugins, that use async heavily in ways that are intended to be client-awaitable—that this would have caught.

This PR:

  • Enables the option at the repo level.
  • Permanently (unless the owners decide to change it) opts out animations and go_router, both of which looked like mostly or entirely false positives.
  • Temporarily opted out a few plugins that have a lot of violations that should be handled in their own PRs later (camera_android_camerax, most of webview_flutter).
  • Fixes all remaining violations.

In many cases this PR is behavior-changing, replacing implicitly unawaited futures that did not seem obviously intentional with awaited futures, so non-test code in particular should be reviewed carefully to make sure the changes are correct. All of the changes are manual, not fix-generated.

Part of flutter/flutter#127323

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use dart format.)
  • I signed the CLA.
  • The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
  • I listed at least one issue that this PR fixes in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
  • I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

Copy link
Contributor

@chunhtai chunhtai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM for go_router and go_router_builder

# TODO(stuartmorgan): Remove this file and fix all the unawaited_futures
# violations. See https://github.com/flutter/flutter/issues/127323

include: ../../../analysis_options.yaml
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated to this pr. I wish we used absolute paths instead of relative paths when the absolute path has more meaning.

I know that effective dart says to prefer relative paths but in this case it matters more that we are using the root analysis_options.yaml than the one 3 folders up.
https://dart.dev/effective-dart/usage#prefer-relative-import-paths

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that would be better in this case, but I don't think it's actually possible; there's no package at the root.

I guess we could make a local package with the shared options, and then depend on that in every single pubspec.yaml so we could use a package: URL, but that would be pretty ugly. And the local package reference in the pubspec would have to be... a relative path.

linter:
rules:
# Matches flutter/flutter, which disables this rule due to false positives.
unawaited_futures: false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mild preference for ignore for file in the test than opting out the directory.

Copy link
Contributor

@tarrinneal tarrinneal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pigeon and shared_prefs are fine, assuming ci doesn't hang on pigeon tests

Copy link
Member

@ditman ditman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why I left some of the calls sneakily unawaited, but CI will tell. 🚀

@stuartmorgan-g
Copy link
Collaborator Author

  • All existing and new tests are passing.

Looks like this was aspirational. Not too many failures though; I'll take a look tomorrow.


test('Injects script into desired target', () async {
loadWebSdk(target: target);
await loadWebSdk(target: target);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change was incorrect; the test never simulates running the callback, causing the test to hang; I'll make it unawaited with a comment.

return (_initialization ??= _doInitialization()).catchError((dynamic _) {
// Invalidate initialization if it errors out.
_initialization = null;
});
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently the fact that the error wasn't being caught by the returned future was (or at least has become) intentional behavior, since there was a unit test that this change broke. I doubt very much that having _initialization not be guaranteed to be null when the future completes is intentional though, so I changed it to return the catchError wrapper, but made the wrapper rethrow after nulling _initialization.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants