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

Conversation

@amirh
Copy link
Contributor

@amirh amirh commented Aug 16, 2019

No description provided.

@amirh amirh requested a review from mklim August 16, 2019 01:38
Copy link
Contributor

@mklim mklim left a comment

Choose a reason for hiding this comment

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

LGTM

/// 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

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

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

Choose a reason for hiding this comment

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

Makes sense.

@amirh amirh merged commit d8eed34 into flutter:master Aug 16, 2019
@amirh amirh deleted the add_comment_on_implements branch August 16, 2019 22:05
sungmin-park pushed a commit to sungmin-park/flutter-plugins that referenced this pull request Dec 17, 2019
julianscheel pushed a commit to jusst-engineering/plugins that referenced this pull request Mar 11, 2020
…tion. (flutter#1982)

* [flutterfire] Mark cloud_firestore and cloud_functions as available for Web in the main feature matrix.
* [cloud_firestore] Add Web integration instructions to README.
* [cloud_firestore_web] Add post-endorsement web integration instructions.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants