Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove the FirebaseOptions() bare initializer. #6633

Merged
merged 3 commits into from
Oct 1, 2020
Merged

Conversation

ryanwilson
Copy link
Member

@ryanwilson ryanwilson commented Oct 1, 2020

This was unintentionally surfaced from the ObjC header and should be
removed.

Note: although the existing initializer doesn't work, this is still a
breaking change.

Fixes #5728

Following recommendations from https://developer.apple.com/videos/play/wwdc2020/10680

This was unintentionally surfaced from the ObjC header and should be
removed.

Note: although the existing initializer doesn't work, this is still a
breaking change.
@google-cla google-cla bot added the cla: yes label Oct 1, 2020
Copy link
Contributor

@maksymmalyhin maksymmalyhin left a comment

Choose a reason for hiding this comment

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

Mostly LGTM, one minor optional question.

@@ -173,6 +173,12 @@ - (id)copyWithZone:(NSZone *)zone {

#pragma mark - Public instance methods

- (instancetype)init {
// Unavailable.
[self doesNotRecognizeSelector:_cmd];
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we really need this? The initializer marked as unavailable should lead to a compiler error, so I guess it should be enough. Or there is a specific case we would like to cover?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, forgot to link! I was watching the "Refine Objective-C frameworks for Swift" WWDC session and that was the suggestion: https://developer.apple.com/videos/play/wwdc2020/10680/?time=1159

They suggested this to make sure there's no accidental calls there. It also surfaced the copyWithZone: implementation calling the init function when it should have likely called one of the designated initializers.

@ryanwilson ryanwilson merged commit 41fe278 into master Oct 1, 2020
@ryanwilson ryanwilson deleted the rw7-options-break branch October 1, 2020 19:43
@firebase firebase locked and limited conversation to collaborators Nov 1, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mark FirebaseOptions empty initializer as NS_UNAVAILABLE
3 participants