[flutter_web_plugins] Migrate to null safety. #69844

ditman · 2020-11-05T01:46:52Z

Description

This PR migrates the flutter_web_plugins to null safety, and solves issues reported by the analyzer.

The functionality stays the same. Null behavior should also stay the same as before; new implementation should be fully backwards-compatible.

Related Issues

Fixes Migrate flutter_web_plugins package to null safety #69843

Tests

Tests are also updated, and they all pass:

$ flutter test --platform chrome

Building flutter tool...
Running "flutter pub get" in flutter_web_plugins...                395ms
Throwing away cached asset graph because the language version of some package(s) changed.
This would most commonly happen when updating dependencies or changing your min sdk constraint.

00:16 +20: All tests passed!

Checklist

Before you create this PR, confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read Handling breaking changes.

No, no existing tests failed, so this is not a breaking change.

packages/flutter_web_plugins/pubspec.yaml

packages/flutter_web_plugins/lib/src/plugin_registry.dart

packages/flutter_web_plugins/lib/src/plugin_event_channel.dart

packages/flutter_web_plugins/analysis_options.yaml

blasten

LGTM after @jonahwilliams comments are addressed

jonahwilliams · 2020-11-05T16:03:33Z

e375651 was reverted, so we need to figure out what the error is before this can land

jonahwilliams · 2020-11-05T16:07:14Z

doing another dependency roll today

packages/flutter_web_plugins/lib/src/navigation/js_url_strategy.dart

packages/flutter_web_plugins/lib/src/navigation/url_strategy.dart

packages/flutter_web_plugins/lib/src/navigation/utils.dart

yjbanov · 2020-11-05T16:47:15Z

packages/flutter_web_plugins/lib/src/navigation/utils.dart

Is ?? '' the right fallback? In the previous version pathname continued to be null. This also ties into the previous question about whether pathname should ever be null.

In this case, pathname comes from dart:html class AnchorElement, and it's defined as String?. In my tests it's never null, in fact, if no pathname is supplied, in chrome it returns '/'. According to the spec, it should return ''.

Looking at the next line of code (which I haven't touched), it seems that pathname will never be null, regardless of what the definition says. I'd say an empty string is a nice fallback.

(Just to double check, you can't call isEmpty on null)

Uncaught TypeError: Cannot read property 'get$isEmpty' of nullError: TypeError: Cannot read property 'get$isEmpty' of null

packages/flutter_web_plugins/lib/src/navigation/utils.dart

packages/flutter_web_plugins/lib/src/plugin_event_channel.dart

jonahwilliams · 2020-11-05T20:06:09Z

Version updates have landed BTW

ditman · 2020-11-05T23:01:04Z

I think I addressed all of @yjbanov's comments above!

@jonahwilliams, thanks for updating the deps! I think this will still fail analysis, because it seems flutter_test is still not migrated (or it seemed so while I was testing this with the latest SDK constraints and without the analysis_options.yaml file) It seems this only is a warning, everything else seems to work as expected. I'll push the change to the pubspec.yaml right now!

jonahwilliams · 2020-11-05T23:41:58Z

flutter and flutter_test should be fully migrated to null safety

packages/flutter_web_plugins/lib/src/navigation/url_strategy.dart

mdebbar

url strategy changes look good to me. Thanks for migrating!

packages/flutter_web_plugins/lib/src/navigation/url_strategy.dart

jonahwilliams · 2020-11-06T00:50:56Z

You need to pull in the latest master

…ovide default values in BrowserPlatformLocation for those.

…on is not null.

…ull safety enabled by default now.

…shState() or replaceState() method is used.

ditman · 2020-11-06T01:33:15Z

Rebased all the way to a3f6ea6.

jonahwilliams

LGTM

kevmoo · 2020-11-09T03:59:51Z

Woo hoo!

When can we expect a dev release cut w/ this commit?

ditman · 2020-11-09T19:41:27Z

@kevmoo I'm not sure about the roll to dev, maybe "Flutter Rollers" knows?

ditman requested review from blasten, jonahwilliams and mdebbar November 5, 2020 01:46

flutter-dashboard bot added c: contributor-productivity Team-specific productivity, code health, technical debt. work in progress; do not review labels Nov 5, 2020

google-cla bot added the cla: yes label Nov 5, 2020

ditman requested review from Hixie and harryterkelsen November 5, 2020 01:47

jonahwilliams reviewed Nov 5, 2020

View reviewed changes

packages/flutter_web_plugins/pubspec.yaml Outdated Show resolved Hide resolved

jonahwilliams reviewed Nov 5, 2020

View reviewed changes

packages/flutter_web_plugins/lib/src/plugin_registry.dart Outdated Show resolved Hide resolved

jonahwilliams reviewed Nov 5, 2020

View reviewed changes

packages/flutter_web_plugins/lib/src/plugin_event_channel.dart Outdated Show resolved Hide resolved

jonahwilliams reviewed Nov 5, 2020

View reviewed changes

packages/flutter_web_plugins/analysis_options.yaml Outdated Show resolved Hide resolved

blasten approved these changes Nov 5, 2020

View reviewed changes

This comment has been minimized.

Sign in to view

yjbanov reviewed Nov 5, 2020

View reviewed changes

This comment has been minimized.

Sign in to view

ditman commented Nov 6, 2020

View reviewed changes

packages/flutter_web_plugins/lib/src/navigation/url_strategy.dart Outdated Show resolved Hide resolved

mdebbar approved these changes Nov 6, 2020

View reviewed changes

packages/flutter_web_plugins/lib/src/navigation/url_strategy.dart Outdated Show resolved Hide resolved

ditman marked this pull request as ready for review November 6, 2020 00:31

ditman removed the work in progress; do not review label Nov 6, 2020

ditman requested review from jonahwilliams and yjbanov November 6, 2020 00:40

Migrate flutter_web_plugins to null safety.

05dddb1

ditman added 10 commits November 5, 2020 17:29

Bring back asserts. Assign TODO to a username.

6520982

Address PR comments wrt setUrlStrategy.

2db9698

Remove commented-out code.

6ca2d57

Make pathname, search and hash non-null in PlatformLocation class. Pr…

de79516

…ovide default values in BrowserPlatformLocation for those.

Eagerly initialize _urlParsingNode, so it can be final and non-null.

211ceb8

Make _baseElement final, and simplify getBaseElementHrefFromDom a lot.

2989548

await directly on subscription?.cancel without checking if subscripti…

95d7c93

…on is not null.

Set the standard sdk constraint. Remove analysis_options.yaml file. N…

73f00f2

…ull safety enabled by default now.

Don't initialize jsUrlStrategy to null. Move constructor to top.

8e27f18

Make state nullable. According to MDN: The value is null until the pu…

dda1e63

…shState() or replaceState() method is used.

jonahwilliams approved these changes Nov 6, 2020

View reviewed changes

ditman changed the title ~~flutter_web_plugins to null safety.~~ Migrate flutter_web_plugins to null safety. Nov 6, 2020

ditman changed the title ~~Migrate flutter_web_plugins to null safety.~~ [flutter_web_plugins] Migrate to null safety. Nov 6, 2020

ditman added the waiting for tree to go green label Nov 6, 2020

fluttergithubbot merged commit e148bf8 into flutter:master Nov 6, 2020

[flutter_web_plugins] Migrate to null safety. #69844

[flutter_web_plugins] Migrate to null safety. #69844

Uh oh!

Conversation

ditman commented Nov 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

blasten left a comment

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

jonahwilliams commented Nov 5, 2020

Uh oh!

jonahwilliams commented Nov 5, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yjbanov Nov 5, 2020

Choose a reason for hiding this comment

Uh oh!

ditman Nov 5, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jonahwilliams commented Nov 5, 2020

Uh oh!

ditman commented Nov 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

jonahwilliams commented Nov 5, 2020

Uh oh!

This comment has been minimized.

Uh oh!

mdebbar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jonahwilliams commented Nov 6, 2020

Uh oh!

ditman commented Nov 6, 2020

Uh oh!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

kevmoo commented Nov 9, 2020

Uh oh!

ditman commented Nov 9, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ditman commented Nov 5, 2020 •

edited

Loading

ditman commented Nov 5, 2020 •

edited

Loading