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

Upgrade SDK, Firebase, null_safety #32

Closed
wants to merge 1 commit into from

Conversation

torstenek
Copy link

Upgraded dependencies to SDK >=2.12.0 and Firebase_core ^1.0.0 along with required transitive dependencies. Works with SDK 2.13.0-77.0.dev

FirebaseImage(
String location, {
this.shouldCache = true,
this.scale = 1.0,
this.maxSizeBytes = 2500 * 1000, // 2.5MB
this.cacheRefreshStrategy = CacheRefreshStrategy.BY_METADATA_DATE,
this.firebaseApp,
required this.firebaseApp,
Copy link
Contributor

Choose a reason for hiding this comment

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

This breaking change should be documented in the change log and/or readme.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this breaking change necessary?

Copy link
Author

Choose a reason for hiding this comment

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

Agreed. firebaseApp is no longer required.

@awhitford
Copy link
Contributor

The top-level pubspec.lock file should be removed (and added to .gitignore). It should not be committed for a library, only for an app.

See https://github.com/toptal/gitignore/blob/2d8160c7f607e142929d2ba213620a638e50603f/templates/Flutter.gitignore#L16

@awhitford
Copy link
Contributor

Actually, I think PR #30 is slightly better because it leverages the late keyword.

@torstenek torstenek marked this pull request as draft March 8, 2021 08:30
@torstenek torstenek force-pushed the null_safety branch 3 times, most recently from 680fab2 to 0c6b87d Compare March 8, 2021 11:03
Upgraded dependencies to SDK >=2.12.0 and Firebase_core ^1.0.0 along with required transitive dependencies. Works with SDK 2.13.0-77.0.dev
@torstenek
Copy link
Author

Using the late keyword is better. Changed it. Sorry for not spotting this in an earlier PR.

@torstenek
Copy link
Author

torstenek commented Mar 8, 2021

I'll leave this PR here since this is the version I'm using, and in the event that it can be useful. I suppose it would be obviated by a merge of PR #30. As with PR #30 it would probably benefit from further testing.

@torstenek torstenek marked this pull request as ready for review March 8, 2021 13:55
@mattreid1
Copy link
Owner

Just merged PR #30. Thanks for your help though!

@mattreid1 mattreid1 closed this Mar 9, 2021
@torstenek
Copy link
Author

torstenek commented Mar 9, 2021 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants