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

[webview_flutter] Comment asking to extend WebViewPlatformController #1982

Merged
amirh merged 1 commit into from
Aug 16, 2019
Merged

[webview_flutter] Comment asking to extend WebViewPlatformController #1982

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions packages/webview_flutter/lib/platform_interface.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,12 @@ abstract class WebViewPlatformCallbacksHandler {
///
/// An instance implementing this interface is passed to the `onWebViewPlatformCreated` callback that is
/// passed to [WebViewPlatformBuilder#onWebViewPlatformCreated].
///
/// Platform implementations that live in a separate package should extend this class rather than
Copy link
Contributor

@mklim mklim Aug 16, 2019

Choose a reason for hiding this comment

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

Do you think there's a case where a platform implementation would ever deliberately want to be broken instead of getting a runtime error method added to them silently? In some cases I could see people preferring to be obviously broken by newer versions instead of having to figure out themselves if there's any new methods that will throw runtime errors. But I'm not sure if that's reasonable.

If you think it's theoretically possible that some people would prefer to be broken at compile time by new versions, I think this should be reworded to something like "Additions to this class aren't treated as breaking changes. Be careful if you choose to implement instead of extend this class as you will be broken by newly added methods instead of getting default implementations."

Copy link
Contributor Author

@amirh amirh Aug 16, 2019

Choose a reason for hiding this comment

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

That's a really good question.

I think that ideally we wouldn't want builds to be broken after upgrading a minor version of a dependency. If we allow platform implementers to implement this class then every addition to this class is a breaking change and requires a major version bump (potentially to the platform interface which may be versioned separately). Technically this is also the case for any method addition to any class.

I see how treating additions to this class as breaking change can make sense, though it's a longer discussion so not dumping my thoughts on this PR.

Copy link
Contributor

@mklim mklim Aug 16, 2019

Choose a reason for hiding this comment

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

Makes sense.

/// implement it as webview_flutter does not consider newly added methods to be breaking changes.
/// Extending this class (using `extends`) ensures that the subclass will get the default
/// implementation, while platform implementations that `implements` this interface will be broken
/// by newly added [WebViewPlatformController] methods.
abstract class WebViewPlatformController {
/// Creates a new WebViewPlatform.
///
Expand Down