Baseflow / flutter_cache_manager

Generic cache manager for flutter
https://baseflow.com
MIT License
749 stars 438 forks source link

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

Open sidrao2006 opened 3 years ago

sidrao2006 commented 3 years ago

🏗 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_inforepositories 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

renefloor commented 3 years ago

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.

sidrao2006 commented 3 years ago

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.

renefloor commented 3 years ago

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.

sidrao2006 commented 3 years ago

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

renefloor commented 3 years ago

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.

sidrao2006 commented 3 years ago

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 commented 3 years ago

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

sidrao2006 commented 3 years ago

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

renefloor commented 3 years ago

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.

sidrao2006 commented 3 years ago

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 commented 3 years ago

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

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.

renefloor commented 2 years ago

@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.

sidrao2006 commented 2 years ago

@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.

monsieurtanuki commented 2 years ago

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.

renefloor commented 2 years ago

@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.

sidrao2006 commented 2 years ago

@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

renefloor commented 2 years ago

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

sidrao2006 commented 2 years ago

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