search

Baseflow / flutter_cache_manager

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

Fix breaking change by hiding File again #321

Closed creativecreatorormaybenot closed 3 years ago

creativecreatorormaybenot commented 3 years ago

:sparkles: What kind of change does this PR introduce? (Bug fix, feature, docs update...)

This fixes the breaking change I explained in https://github.com/Baseflow/flutter_cache_manager/pull/302#issuecomment-853120621.

:arrow_heading_down: What is the current behavior?

When you run pub upgrade with flutter_cache_manager: ^3.0.0 and used 3.0.0 before, your code breaks because you might have imported dart:io (or universal_io) and flutter_cache_manager in the same file.

:new: What is the new behavior (if this is a feature change)?

Removed the export of package:file. Instead, users can import it when needed.

What should be done instead?

@sidrao2006 said that you have to add a direct package:file depedendency if you want to annotate types without his PR (this reverts #302), however, that is not true.

You can import package:file/file.dart, even when it is a transitive dependency.

Migration

So the migration is simply adding:

import 'package:file/file.dart' show File;

Whenever you want to use that File class.

:boom: Does this PR introduce a breaking change?

Technically yes, but it fixes the previous breaking change.

:thinking: Checklist before submitting

sidrao2006 commented 3 years ago

@creativecreatorormaybenot for some reason importing transitive dependencies isn't allowed in packages

creativecreatorormaybenot commented 3 years ago

@sidrao2006 ok, it makes sense in packages. But in that case I would prefer to make the dependency explicitly visible, or not? 🤔

The reason I said it was a breaking change is that in our apps, it would not compile anymore, unless we add:

import 'package:flutter_cache_manager/flutter_cache_manager.dart' hide File;

Because of the mentioned conflict.

sidrao2006 commented 3 years ago

I completely understand and apologize for the change. When I made the change, the dart:io conflict didn't occur to me. Sorry for the inconvenience caused.

creativecreatorormaybenot commented 3 years ago

@sidrao2006 No worries. The change itself is fine as well (although I would say it generally does not make most sense to export other packages from your package), but it should have bumped the major.

renefloor commented 3 years ago

Slightly different fix from PR #324 is released as 3.1.1

creativecreatorormaybenot commented 3 years ago

@renefloor I thought about this approach as well, but I think it is a bad idea.

You are essentially proxying import 'package:file/file.dart (so you have a library in your package which has the sole purpose of proxying another library). The reason this is also dangerous is that users will not have an explicit dependency on file, which means that they can be hit by breaking changes without being able to know why.

Now, you are required to publish a breaking change / major version update whenever file has a major version update / breaking change because you are exporting it.

sidrao2006 commented 3 years ago

@creativecreatorormaybenot , only File is exported from the package. So, a major version would have to be published only if there are massive changes in File which is highly unlikely. Even in such a case, several major refactors would have to be made in flutter_cache_manageras well. So, then it would make sense to release a major version (or minor, as needed)

creativecreatorormaybenot commented 3 years ago

@sidrao2006 yes that is true.

renefloor commented 3 years ago

Indeed, if for example file.read() would change then the cache_manager package needs a breaking change anyhow. In general it is not a good idea to export other packages, but it's also weird that you get a File, but can only do var file =.. and not File file = ... I'm also considering migrating to cross_file, but I'll have to study the differences.