Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.

[webview_flutter] Migrate main package to fully federated architecture. #4366

Merged
merged 13 commits into from
Sep 23, 2021
Merged

[webview_flutter] Migrate main package to fully federated architecture. #4366

merged 13 commits into from
Sep 23, 2021

Conversation

mvanbeusekom
Copy link
Contributor

This PR migrates the main "app facing" webview_flutter package to depend on the new webview_flutter_platform_interface, webview_flutter_android and webview_flutter_wkwebview packages. This finalises the migration to the fully federated architecture as described in issue flutter/flutter#86286.

Relevant issue:

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. (Note that unlike the flutter/flutter repo, the flutter/plugins repo does use dart format.)
  • I signed the CLA.
  • The title of the PR starts with the name of the plugin 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.
  • I updated CHANGELOG.md to add a description of the change.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@google-cla google-cla bot added the cla: yes label Sep 21, 2021
@github-actions github-actions bot added p: webview_flutter Edits files for a webview_flutter plugin platform-android platform-ios labels Sep 21, 2021
stuartmorgan-g
stuartmorgan-g reviewed Sep 21, 2021
View reviewed changes
Copy link
Contributor

@stuartmorgan-g stuartmorgan-g left a comment

Choose a reason for hiding this comment

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

Code changes LG, but the iOS test failure looks real since it's across both channels. Any idea what's going on there?

@mvanbeusekom
Remove native Android implementation.
702dc21 
Removed the native Android code as first step to migrate to the fully
federated plugin architecture.
@mvanbeusekom
Remove Android specific Dart code.
4020ee9 
Removed all Android specific Dart code that will be replaced by the
webview_flutter_android package.
@mvanbeusekom
Remove native iOS implementation.
899bbe1 
Removed the native iOS implementation as this will be replaced by the
implementation in the webview_flutter_wkwebview package.
@mvanbeusekom
Remove iOS specific Dart code.
f29fe3a 
Removed all iOS specific Dart code as it will be replaced by the
webview_flutter_wkwebview package.
@mvanbeusekom
Remove platform interface related code.
157b696 
Removed the `lib/src/webview_method_channel.dart` and
`lib/platform_interface.dart` files as they will be replaced by the
implementations from the webview_flutter_platform_interface package.
@mvanbeusekom
Add dependencies to pubspec.yaml.
c1043d8 
Update pubspec.yaml with configuration to depend on the
webview_flutter_platform_interface, webview_flutter_android and
webview_flutter_wkwebview packages.
@mvanbeusekom
Re-export platform interface classes.
7818e52 
Added the platform_interface.dart file which re-exports the public
classes from the webview_flutter_platform_interface package so we don't
break existing non-endorsed implementations of the old
platform_interface.
@mvanbeusekom
Move implementation to lib/src/webview.dart
e3f3946 
Direct copy of the existing implementation from
lib/webview_flutter.dart to a new implementation file
at location lib/src/webview.dart. This will open the way to add a bundle
file which will export all important parts over all of the federated
pakcages.
@mvanbeusekom
Update code to depend on webview_flutter_platform_interface.
4274bba 
Update the implementation of the lib/src/webview.dart file so it
correctly depends on the webview_flutter_platform_interface package
(imported via the platform_interface.dart file).
@mvanbeusekom
New bundle file to expose federated packages.
71125f6
@mvanbeusekom
Fix all tests.
a2c33bd 
Fixes all tests to import the correct dependencies.
@mvanbeusekom
Fixed analysis warning.
3fd17b8 
Removed unused variable from the _WebViewState.
@mvanbeusekom
Ensure SurfaceAndroidWebView tests don't run on iOS.
dfd4d35
@mvanbeusekom mvanbeusekom force-pushed the webview/federated_architecture_part_5 branch from fc432b6 to dfd4d35 Compare September 22, 2021 09:11
@mvanbeusekom
Copy link
Contributor Author

Code changes LG, but the iOS test failure looks real since it's across both channels. Any idea what's going on there?

That was indeed a mistake from my side. The SurfaceAndroidWebView integration tests were also run on iOS because I forgot to switch the _skipDueToIssue86757 back to true (I had set it to false to run tests locally). I have made skipping these tests a bit more robuust and added an additional condition so they only run when Platform.isAndroid is true.

stuartmorgan-g
stuartmorgan-g approved these changes Sep 22, 2021
View reviewed changes
Copy link
Contributor

@stuartmorgan-g stuartmorgan-g left a comment

Choose a reason for hiding this comment

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

LGTM

@mvanbeusekom mvanbeusekom added the waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land. label Sep 23, 2021
@fluttergithubbot fluttergithubbot merged commit 090e406 into flutter:master Sep 23, 2021
@mvanbeusekom mvanbeusekom mentioned this pull request Sep 23, 2021
Merged
11 tasks
@mvanbeusekom mvanbeusekom deleted the webview/federated_architecture_part_5 branch September 23, 2021 15:20
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Sep 23, 2021
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 23, 2021
@engine-flutter-autoroll
090e406 [webview_flutter] Migrate main package to fully federated arc…
8860903 
…hitecture. (flutter/plugins#4366)
amantoux pushed a commit to amantoux/plugins that referenced this pull request Sep 27, 2021
@mvanbeusekom @amantoux
[webview_flutter] Migrate main package to fully federated architectur…
6d25e0f 
…e. (flutter#4366)
NickalasB added a commit to NickalasB/plugins that referenced this pull request Sep 27, 2021
@NickalasB
Merge branch 'master' into add_zoom_to_android_webview
8dd870b 
* master: (51 commits)
  [webview_flutter] Update version number app_facing package (flutter#4375)
  [webview_flutter] Adjust integration test domains (flutter#4383)
  Remove some trivial custom analysis options files (flutter#4379)
  [google_maps_flutter_platfomr_interface] Add Marker drag events (flutter#2653)
  [flutter_plugin_tools] Improve version check error handling (flutter#4376)
  [flutter_plugin_tools] Allow overriding breaking change check (flutter#4369)
  [url_launcher] Error handling when URL cannot be parsed with Uri.parse (flutter#4365)
  [webview_flutter] Migrate main package to fully federated architecture. (flutter#4366)
  [google_sign_in] Bump minimum Flutter version and iOS deployment target (flutter#4334)
  Add false secret lists, and enforce ordering (flutter#4372)
  [camera_web] Update usage documentation (flutter#4371)
  [video_player] VTT Support (flutter#2878)
  Require authors file (flutter#4367)
  [flutter_plugin_tools] Fix federated safety check (flutter#4368)
  [webview_flutter] Extract WKWebView implementation into a separate package (flutter#4345)
  [webview_flutter] Extract Android implementation into a separate package (flutter#4343)
  [in_app_purchase] Ensure the `introductoryPriceMicros` field is populated correctly. (flutter#4364)
  [flutter_plugin_tools] Add a federated PR safety check (flutter#4329)
  [camera] Add web support (flutter#4240)
  [webview_flutter] Bump minimum Flutter version and iOS deployment target (flutter#4361)
  ...

# Conflicts:
#	packages/webview_flutter/webview_flutter/lib/platform_interface.dart
#	packages/webview_flutter/webview_flutter/lib/src/webview_method_channel.dart
#	packages/webview_flutter/webview_flutter/lib/webview_flutter.dart
stuartmorgan-g
stuartmorgan-g reviewed Oct 6, 2021
View reviewed changes
packages/webview_flutter/webview_flutter/lib/platform_interface.dart
JavascriptMessage,
JavascriptMode,
JavascriptMessageHandler,
WebViewPlatform,
Copy link
Contributor

Choose a reason for hiding this comment

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

For posterity: we accidentally shipped a breaking change to external implementors here without noticing. There's nothing we can reasonably do about it at this point, and AFAIK it only affected one client (now fixed), but what happened was:

Because in this repository we previously had only the one combined package, and we were thus not eating our own dogfood in an important way with respect to the platform interface, nothing in CI alerted us to the mistake. The good news is that being fully federated gives us much more protection against making that mistake in the future (which is validation of the need for us to follow the intended policy of having all of our federated plugins be fully federated precisely so that we are eating our own dogfood).

So ironically the change that will save us from this bug in the future caused exactly the bug it's meant to avoid. But the bug was one-time, and the protection is persistent for all future changes, so it's definitely a net win despite that.

KyleFin pushed a commit to KyleFin/plugins that referenced this pull request Dec 21, 2021
@mvanbeusekom @KyleFin
[webview_flutter] Migrate main package to fully federated architectur…
333f8b7 
…e. (flutter#4366)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@stuartmorgan-g stuartmorgan-g stuartmorgan-g approved these changes

Assignees
No one assigned
Labels
cla: yes p: webview_flutter Edits files for a webview_flutter plugin platform-android platform-ios waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[webview_flutter] Migrate plugin to federated architecture
3 participants
@mvanbeusekom @stuartmorgan-g @fluttergithubbot