flutter_cache_manager icon indicating copy to clipboard operation
flutter_cache_manager copied to clipboard

Isolate imports and conditionally import libraries to make package platform-independent

Open sidrao2006 opened this issue 3 years ago • 18 comments

🏗 Enhancement Proposal

Currently flutter_cache_manager is platform-independent, yet according to pub.dev, the package doesn't support web. This can be solved using conditionally importing dart:io dependent libraries. path_provider, file and sqflite are platform-dependent and must only be imported/exported in *_io.dart files which will then be conditionally imported/exported.

file can be replaced with cross_file (as mentioned in #321 (comment))

path_provider and sqflite are rather easy to isolate since they are imported only in cache_info_repositories which are selectively imported in config_*.dart files. The only problem is that cache_info_repositories/cache_info_repositories.dart is exported. @renefloor what can we do about this?

Pitch

This will increase pub points which affect discoverability. Also, partially platform-dependent code will be separated and be easier to maintain.

Checklist

  • [ ] #339
  • [ ] Store the config json data as a file on all platforms except the web, where web storage will be used to store this data
  • [ ] Make the JSON implementation of cache info repository platform independent
  • [ ] Migrate all instances to use the new JSON implementation
  • [ ] If necessary, replace platform dependent dependencies with cross-platform implementations

sidrao2006 avatar Jun 17 '21 15:06 sidrao2006

I think this is a good idea. I have been trying this already and did a lot of restructuring work. That's also the main reason the Config file is created to easily create a separate config for io and for web and not having to do the conditional import for all classes. However, I was mainly struggling with replacing the directory structure, but I made a start with that as well by creating the FileSystem class.

One of the reasons I did not continue working on it (besides I was kind of done with it) is that when pub.dev shows the package supports web devs might expect full caching support on web, but I've not found a good way to really support web besides just downloading the file.

renefloor avatar Jun 21 '21 06:06 renefloor

web devs might expect full caching support on web, but I've not found a good way to really support web besides just downloading the file

@renefloor could you please elaborate on that?

I did notice that NonStoringObjectProvideris used for web and it has no implementation.

sidrao2006 avatar Jun 21 '21 06:06 sidrao2006

Yes, so at the moment this CacheManager doesn't cache anything on the web. Replacing the database with cache information is relatively easy, so example hive uses an indexedDB. However, storing the files themselves is a bit more difficult. We already had a pretty long discussion with some small promise of how it might work here: #122.

renefloor avatar Jun 21 '21 07:06 renefloor

Hmm.. And what about Windows and Linux? We could use sqflite_ffi

sidrao2006 avatar Jun 21 '21 07:06 sidrao2006

Windows and Linux are already compatible because those don't use sqflite, but they use a JSON file by default. I want to migrate all plaforms to using a JSON file by default and move sqflite to a separate package. I'm not sure if there is a way to do a conditional import based on platform to already fix this without the full migration. I think conditional imports are always io vs html.

renefloor avatar Jun 21 '21 08:06 renefloor

You read my mind! I was about to suggest migrating all platforms to use a json file.. Have you looked into universal_io?

You could simply merge the two config files into one which uses the JSON implementation and replace dart:io with universal_io

sidrao2006 avatar Jun 21 '21 08:06 sidrao2006

As for path_provider, we can do a conditional import with a custom implementation for web

sidrao2006 avatar Jun 21 '21 08:06 sidrao2006

@renefloor , would you be interested in rewriting the implementation for file system and using JSON files by default?

sidrao2006 avatar Sep 29 '21 17:09 sidrao2006

That's already implemented, only the default has to switch and the sqflite version migrated. However you still have to get rid of dart:io for the MemoryFileSystem that is currently used for web. I haven't had a look yet if Universal io can replace that.

renefloor avatar Sep 29 '21 18:09 renefloor

Hmm.. Right

But we would still have to get rid of the dart:io, path and path_provider imports in the JSON implementation. In short, we'd need the JSON implementation to be platform independent.

So how about using Indexeddb to store files and localstorage to store the JSON data for the Web?

sidrao2006 avatar Sep 30 '21 05:09 sidrao2006

So @renefloor, the main steps should be something like this -

  • Implement file_system for the web using IndexedDB to store the files (more specifically, the file content as blobs)
  • Store the config json data as a file on all platforms except the web, where web storage will be used to store this data
  • Make the JSON implementation of cache info repository platform independent
  • Migrate all instances to use the new JSON implementation
  • If necessary, replace platform dependent dependencies with cross-platform implementations

If you are satisfied with the above mentioned changes, I'll go ahead and create separate issues and PRs to track each change and use this issue to track the overall migration to cross-platform implementations.

sidrao2006 avatar Oct 03 '21 09:10 sidrao2006

@sidrao2006 this sounds like a good idea. I think we can also store the json file in the indexesDB? 1 thing I'm not sure about if whether we know how large the storage is and if we can do anything with that. This might also be a good improvement for the native implementation, but I think we might have to check for free storage and if that's to low delete the oldest file.

renefloor avatar Oct 04 '21 06:10 renefloor

@renefloor it'd be a bit more performant to store the json data from the json file using local storage but we could definitely use indexddb too..

navigator.storage.estimate() provides a way to check the total size of the indexeddb. I'm not sure about checking for free storage on the web but the above also provides a 'quota' field which lists the total storage allowed to be used by the website.

sidrao2006 avatar Oct 04 '21 09:10 sidrao2006

Coming from https://github.com/openfoodfacts/smooth-app/issues/413...

Yeah true. But I also kind of want to get rid of the sqflite implementation in the code and move it to a separate package, so I want to make it as easy and clear as possible. But that is indeed the tricky part.

@renefloor I don't know how you could migrate from sqflite to json without sqflite. That means you need to import sqflite, just in case. From what I read in the README, you are pretty relaxed about the cache being deleted, as it's in a temporary directory.

Here another suggestion: during a grace period (let's say 6 months?) you keep sqflite - with a deprecation warning - and you give the developers the nudged possibility to migrate from sqlfite to json. Then 6 months later you switch to a breaking MAJOR release, where it's sqflite-free and 100% json. Worst case scenario: developers that did not migrate will start again from an empty cache, which is a possibility anyway when the temporary directories are cleaned.

monsieurtanuki avatar Oct 27 '21 10:10 monsieurtanuki

@monsieurtanuki I agree about that. Currently we are on version 3. I'm thinking about creating version 4 that migrates to json and after that version 5 removes the sqflite completely. Then people can also decide to stay at version 4.

renefloor avatar Oct 27 '21 10:10 renefloor

@renefloor, do you think we could 'auto migrate' to JSON? v4 would have the JSON implementation as the default but we'll have a dependency on sqflite to read the dB and transfer its contents to our json file, thereby migrating the app to our new JSON implementation.

P.S. Still waiting for your reply about the file system problem in https://github.com/Baseflow/flutter_cache_manager/issues/339#issuecomment-941209264

sidrao2006 avatar Oct 27 '21 10:10 sidrao2006

We can auto migrate, but I'm not sure if we should.

renefloor avatar Oct 27 '21 10:10 renefloor

I think the best way would be to provide the option to auto migrate and also an indication if the migration has already been performed.

Since v5 should come out shortly after publishing v4, developers will have the option to see if the majority of their user's have already migrated and only then move on to v5

sidrao2006 avatar Oct 27 '21 11:10 sidrao2006