Migrate to NNBD #278

Jjagg · 2021-02-11T22:08:01Z

This migrates the library to NNBD.
For now this only migrates the flutter_cache_manager package and its tests.
There's 2 test failures I have yet to resolve, so I'm submitting as a draft for now.

Heads up that build_runner needs to be run before running tests because of changes to Mockito to support NNBD.

Fixes #253.

🤔 Checklist before submitting

All projects build
Follows style guide lines (code style guide)
Relevant documentation was updated
Rebased onto current develop

Jjagg · 2021-02-11T22:09:47Z

flutter_cache_manager/lib/src/cache_store.dart

+          _memCache.remove(key);
+        } else {
+          _memCache[key] = cacheObject;
+        }


Behavior change here. Seemed to make sense to me to remove null values and make the cached items non-nullable.

Jjagg · 2021-02-11T22:12:54Z

flutter_cache_manager/lib/src/result/file_info.dart

@@ -22,5 +22,5 @@ class FileInfo extends FileResponse {

  /// Validity date of the file. After this date the validity is not guaranteed
  /// and the CacheManager will try to update the file.
-  final DateTime validTill;
+  final DateTime? validTill;


I don't know if this actually should be null. Had to use a lot of bangs because I made it nullable.

I think it's better to make it required.

Jjagg · 2021-02-11T22:17:41Z

flutter_cache_manager/test/helpers/mock_cache_info_repository.dart

+    when(provider.open()).thenAnswer((_) => Future.value(true));
+    when(provider.update(any, setTouchedToNow: anyNamed('setTouchedToNow')))
+        .thenAnswer((realInvocation) => Future.value(0));
+    when(provider.updateOrInsert(any)).thenAnswer((realInvocation) async =>


You can let Mockito return null for unstubbed methods, but I did not enable that because it can cause runtime casting errors with NNBD. Downside is all methods need to be stubbed explicitly.

Jjagg · 2021-02-11T22:19:25Z

flutter_cache_manager/test/mock.mocks.dart

+              }),
+              returnValue: Stream<_i10.FileResponse>.empty())
+          as _i7.Stream<_i10.FileResponse>);
+}


You might prefer not having this generated file checked in, LMK.

flutter_cache_manager/lib/src/cache_managers/default_cache_manager.dart

hoc081098 · 2021-02-18T04:53:46Z

Dart beta has been already release, please publish it. https://medium.com/dartlang/preparing-the-dart-and-flutter-ecosystem-for-null-safety-e550ce72c010

flutter_cache_manager/analysis_options.yaml

flutter_cache_manager/lib/src/cache_manager.dart

Jjagg · 2021-02-18T16:41:29Z

Thanks for reviewing @hoc081098, I made some changes.

renefloor · 2021-02-19T10:53:27Z

@Jjagg I wanted to start with this today, but found your PR.
I think we should have way more non-optionals and throw an exception if something is not found.
For example

Future<FileInfo?> getFile(String key, {bool ignoreMemCache = false})

This should just be Future<FileInfo?> imo. What do you prefer? Shall I comment on your PR, push changes to your branch or start my own branch?

I don't mind any breaking changes, as long as it is documented. That's why this will be a new major version.

Jjagg · 2021-02-19T11:16:52Z

IMO a caching library should not throw an exception when an item is retrieved that is not in the cache. But I don't have too strong of an opinion on it and I don't use this library directly, so it doesn't matter much to me.

I've given you access to my fork. Feel free to commit directly to this branch or branch off and set up your own pull request/commit directly to the main branch. Either would be a little faster than you commenting and me making the changes :)

renefloor · 2021-02-19T11:55:50Z

I'm also not 100% sure about the best way, I think both null and an exception are fine if documented well.

ryanheise · 2021-02-19T12:06:21Z

From Effective Dart:

Dart uses exceptions when an error occurs in your program.

And from the Language Tour:

Exceptions are errors indicating that something unexpected happened

I agree that for a caching library, a cache miss is not an error but something to be expected.

acoutts · 2021-02-23T18:59:11Z

Hi- the force unwrap here is causing a runtime error for me:
https://github.com/Jjagg/flutter_cache_manager/blob/nnbd/flutter_cache_manager/lib/src/storage/file_system/file_system_io.dart#L16

../../.pub-cache/git/flutter_cache_manager-439af02c4c059986f46101a003ac8ebd20658ef7/flutter_cache_manager/lib/src/storage/file_system/file_system_io.dart:16:23: Warning: Operand of null-aware operation '!' has type 'Directory' which excludes null.
 - 'Directory' is from 'dart:io'.
    var path = p.join(baseDir!.path, key);
                      ^

../../.pub-cache/git/flutter_cache_manager-439af02c4c059986f46101a003ac8ebd20658ef7/flutter_cache_manager/lib/src/storage/cache_info_repositories/cache_object_provider.dart:196:20: Warning: Operand of null-aware operation '!' has type 'Directory' which excludes null.
 - 'Directory' is from 'dart:io'.

      directory = (await getApplicationSupportDirectory())!;
                  ^

acoutts · 2021-02-23T19:00:28Z

Jjagg · 2021-02-23T23:05:32Z

@acoutts
Looks like they changed behavior in path_provider for the the stable 2.0.0 release: flutter/plugins#3582.
Are you overriding that dependency by any chance?

I'll update dependencies to the stable version for packages where it's already available and fix that issue.

acoutts · 2021-02-23T23:26:27Z

I'm not overriding it no- i'm on 2.0.0-nullsafety.1.

acoutts · 2021-02-24T13:13:58Z

I was able to silence the error with this override:

dependency_overrides:
  path_provider: 2.0.0-nullsafety.1

# Conflicts: # flutter_cache_manager/lib/src/storage/cache_info_repositories/json_cache_info_repository.dart

renefloor · 2021-02-25T09:43:16Z

In the latest stable of path_provider they changed the return type from Future<Directory?> to Future
flutter/plugins@bb3fc5a#diff-78e86d7326b605de0b626343cc28963ef7bff3587995640f6c1dddadbd7333ed

I upgraded the dependencies that have a stable version now.

renefloor · 2021-02-25T13:56:27Z

flutter_cache_manager/test/cache_manager_test.dart

+    // test('Non-existing cacheFile should call to web', () async {
+    //   var fileName = 'test.jpg';
+    //   var fileUrl = 'baseflow.com/test';
+    //   var fileKey = 'test1234';
+    //   var validTill = DateTime.now().subtract(const Duration(days: 1));
+    //
+    //   var config = createTestConfig();
+    //   config.returnsCacheObject(fileUrl, fileName, validTill, key: fileKey);
+    //
+    //   var cacheManager = TestCacheManager(config);
+    //
+    //   var result = await cacheManager.getSingleFile(fileUrl, key: fileKey);
+    //   expect(result, isNotNull);
+    //   config.verifyDownloadCall();
+    // });


@Jjagg I couldn't get this test working, do you have any idea what's going wrong here? It looks like webhelper.downloadFile is not really called, but that is not mocked.

What was the error message?

From your comment I think it's because of the changes to mockito with NNBD.
Mocks are generated with build_runner and by default all methods throw. Any method that is called should be mocked.

kevmoo

Thanks for this! Would love to get this landed. This is on our list of critical/important community packages!

Let me know if you need any help!

kevmoo · 2021-02-28T00:37:59Z

flutter_cache_manager/CHANGELOG.md

@@ -1,3 +1,7 @@
+## [3.0.0-nullsafety.0] - 2021-02-25


Let's drop the -nullsafety.0 bits. We can release stable now!

See https://medium.com/dartlang/preparing-the-dart-and-flutter-ecosystem-for-null-safety-e550ce72c010

True, but besides null safety being stable we also make sure that this package is stable.
All tests are stil broken and @ryanheise also found a bug. So we can change 3.0.0-beta.0, but that doesn't really help much.

kevmoo · 2021-02-28T00:38:37Z

flutter_cache_manager/pubspec.yaml

@@ -1,26 +1,28 @@
 name: flutter_cache_manager
 description: Generic cache manager for flutter. Saves web files on the storages of the device and saves the cache info using sqflite.
-version: 2.1.1
+version: 3.0.0-nullsafety.0


drop the -nullsafety bit here. Should be ready for stable!

kevmoo · 2021-02-28T00:39:00Z

flutter_cache_manager/pubspec.yaml

-  image: ^2.1.18
-
+  http: ^0.13.0
+  image: ^3.0.0-nullsafety.0


drop -nullsafety this is published stable now!

kevmoo · 2021-02-28T00:39:08Z

flutter_cache_manager/pubspec.yaml

+  path: ^1.8.0
+  path_provider: ^2.0.0
+  pedantic: ^1.10.0
+  rxdart: ^0.26.0-nullsafety.1


drop -nullsafety this is published stable now!

kevmoo · 2021-02-28T20:00:27Z

Who has the power to merge and publish?

Jjagg · 2021-02-28T20:03:10Z

That would be @renefloor :)

renefloor · 2021-02-28T20:09:08Z

@kevmoo it is already published as preview version: https://pub.dev/packages/flutter_cache_manager/versions/3.0.0-nullsafety.0

I try to migrate CachedNetworkImage this week and if I don't get any issues I'll merge and publish as stable.

ryanheise · 2021-03-01T15:31:49Z

In NonStoringObjectProvider, it defines:

  @override
  Future<CacheObject> get(String url) {
    return Future.value(null);
  }

But CacheObject is non-nullable.

The analyser didn't detect any problem statically (which is unfortunate since that should be the goal of null safety) but the documentation for Future.value, indicates that you will get a runtime exception:

If [value] is omitted or null, it is converted to FutureOr<T> by value as FutureOr<T>. If T is not nullable, then a non-null [value] must be provided, otherwise the construction throws.

PcolBP · 2021-03-16T11:12:07Z

After upgrading sqflite to 2.0.0+3 , now getDatabasesPath returns non-nullable String no need to add null-aware operand.

In case when sqflite is upgraded and cacheManager stays at nullsafety.1 branch error occur:

Jjagg · 2021-03-16T11:55:29Z

I'm only getting a warning, so they might have changed the analyzer behavior. Nevertheless, doesn't hurt to upgrade sqflite.
EDIT: Oh, I thought you got an error because of the red text 😅

# Conflicts: # flutter_cache_manager/CHANGELOG.md # flutter_cache_manager/example/pubspec.yaml # flutter_cache_manager/pubspec.yaml

davidmartos96 · 2021-03-23T10:05:25Z

@Jjagg There is this issue as well in the NNBD version. #297
The id of the cache object can be null when reaching the removeFile function, so the unconditional bang fails.

codecov · 2021-03-26T16:22:25Z

Codecov Report

Merging #278 (c6c6fb6) into develop (d9061bd) will decrease coverage by 1.37%.
The diff coverage is 72.26%.

@@             Coverage Diff             @@
##           develop     #278      +/-   ##
===========================================
- Coverage    76.11%   74.73%   -1.38%     
===========================================
  Files           21       21              
  Lines          674      669       -5     
===========================================
- Hits           513      500      -13     
- Misses         161      169       +8

Impacted Files	Coverage Δ
.../lib/src/cache_managers/default_cache_manager.dart	`0.00% <0.00%> (ø)`
...ter_cache_manager/lib/src/compat/file_fetcher.dart	`77.77% <ø> (-12.23%)`	⬇️
...utter_cache_manager/lib/src/config/_config_io.dart	`12.50% <ø> (ø)`
...cache_info_repositories/cache_object_provider.dart	`0.00% <0.00%> (ø)`
...info_repositories/non_storing_object_provider.dart	`0.00% <ø> (ø)`
...er/lib/src/storage/file_system/file_system_io.dart	`0.00% <0.00%> (ø)`
flutter_cache_manager/lib/src/web/queue_item.dart	`100.00% <ø> (ø)`
..._info_repositories/json_cache_info_repository.dart	`90.62% <75.86%> (-2.16%)`	⬇️
...lutter_cache_manager/lib/src/web/file_service.dart	`87.87% <81.81%> (-6.57%)`	⬇️
flutter_cache_manager/lib/src/cache_manager.dart	`88.60% <83.33%> (+1.42%)`	⬆️
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c869918...c6c6fb6. Read the comment docs.

Jjagg added 2 commits February 11, 2021 17:04

Migrate flutter_cache_manager lib to NNBD

6ab2806

Migrate flutter_cache_manager tests to NNBD

0e086ec

Jjagg commented Feb 11, 2021

View reviewed changes

Jjagg mentioned this pull request Feb 11, 2021

Support NNBD Baseflow/flutter_cached_network_image#503

Closed

2 tasks

ryanheise reviewed Feb 12, 2021

View reviewed changes

flutter_cache_manager/lib/src/cache_managers/default_cache_manager.dart Outdated Show resolved Hide resolved

hoc081098 reviewed Feb 18, 2021

View reviewed changes

flutter_cache_manager/analysis_options.yaml Outdated Show resolved Hide resolved

flutter_cache_manager/lib/src/cache_manager.dart Show resolved Hide resolved

Fix oopsies

439af02

ryanheise mentioned this pull request Feb 24, 2021

Null safety ryanheise/audio_service#536

Closed

renefloor added 2 commits February 25, 2021 10:36

update dependencies

08b32e2

Merge remote-tracking branch 'origin/develop' into nnbd

232e775

# Conflicts: # flutter_cache_manager/lib/src/storage/cache_info_repositories/json_cache_info_repository.dart

renefloor added 3 commits February 25, 2021 11:13

Make relativePath and validTill required

b2a1148

make tests compile

0f0209f

Fix tests and disabled one

f5ccd30

renefloor reviewed Feb 25, 2021

View reviewed changes

renefloor marked this pull request as ready for review February 25, 2021 13:56

renefloor approved these changes Feb 25, 2021

View reviewed changes

renefloor added 5 commits February 25, 2021 15:44

Fix analysis warnings

a674f46

Format test and lib

6c1c35d

Removed nullability of validTill

ea67f1e

Removed one more null check

272d6a4

make package publishable

fe79974

kevmoo reviewed Feb 28, 2021

View reviewed changes

Stable nullsafety

11834a1

Fix get in NonStoringObjectProvider

ac346c0

renefloor mentioned this pull request Mar 10, 2021

_removeCachedFile Null check operator used on a null value #297

Closed

2 tasks

Update sqflite

bf34840

PcolBP mentioned this pull request Mar 16, 2021

on every start/restart warning: Operand of null-aware operation #299

Closed

Set publish_to: none for example to prevent warnings

dc33eaf

renefloor added 2 commits March 22, 2021 08:06

Merge branch 'develop' into nnbd

1815a74

# Conflicts: # flutter_cache_manager/CHANGELOG.md # flutter_cache_manager/example/pubspec.yaml # flutter_cache_manager/pubspec.yaml

Update version

ab192c6

renefloor added 2 commits March 26, 2021 16:22

Fix Baseflow#297

4eeb029

Test id to fix test

c6c6fb6

renefloor merged commit 776fb13 into Baseflow:develop Mar 27, 2021

Uh oh!

Migrate to NNBD #278

Migrate to NNBD #278

Uh oh!

Conversation

Jjagg commented Feb 11, 2021

🤔 Checklist before submitting

Uh oh!

Jjagg Feb 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hoc081098 commented Feb 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Jjagg commented Feb 18, 2021

Uh oh!

renefloor commented Feb 19, 2021

Uh oh!

Jjagg commented Feb 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

renefloor commented Feb 19, 2021

Uh oh!

ryanheise commented Feb 19, 2021

Uh oh!

acoutts commented Feb 23, 2021

Uh oh!

acoutts commented Feb 23, 2021

Uh oh!

Jjagg commented Feb 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

acoutts commented Feb 23, 2021

Uh oh!

acoutts commented Feb 24, 2021

Uh oh!

renefloor commented Feb 25, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevmoo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevmoo commented Feb 28, 2021

Uh oh!

Jjagg commented Feb 28, 2021

Uh oh!

renefloor commented Feb 28, 2021

Uh oh!

ryanheise commented Mar 1, 2021

Uh oh!

PcolBP commented Mar 16, 2021

Uh oh!

Jjagg commented Mar 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Jjagg Feb 11, 2021 •

edited

Loading

hoc081098 commented Feb 18, 2021 •

edited

Loading

Jjagg commented Feb 19, 2021 •

edited

Loading

Jjagg commented Feb 23, 2021 •

edited

Loading

Jjagg commented Mar 16, 2021 •

edited

Loading

codecov bot commented Mar 26, 2021 •

edited

Loading